Introduce lock screen app context to extension features

Introduce lock screen app context to extension features

The context will be used for platform apps that are runnable
on Chrome OS lock screen - the context will be derived from the
containing browser process. Apps running in sign-in profile after
the user was logged in will run in 'lock screen' script context.
Because sign-in profile is Chrome concept, unknown to extensions
layer, the information will have to be relayed to extensions system.
The script context is determined in two places:
* ScriptContextSet, which determines script context used in the render 
  process. The script context set will be flagged as lock screen 
  context during renderer initialization - it will be sent from the 
  browser process in SetSessionInfo IPC message.
* ProcessMap, used in browser process. ProcessMaps are created per 
  browser context - process maps belonging to sign-in profile will be
  marked as lock screen context process map (as before that point, the 
  sign-in profile will be used for login)

BUG=715781
Review-Url: https://codereview.chromium.org/2892403002
Cr-Commit-Position: refs/heads/master@{#478506}
Committed: https://chromium.googlesource.com/chromium/src/+/8e89b0b1a1bd64d016a24a182a715aeedb712469

[tbarzic, 2017-05-22 03:25:10]

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

[commit-bot: I haz the power, 2017-05-22 03:25:19]

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-05-22 04:02:59]

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

[commit-bot: I haz the power, 2017-05-22 04:03:00]

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

[tbarzic, 2017-05-22 23:16:41]

Description was changed from

==========
Introduce lock screen extension context to extension features

The context will be used for platform apps running that runnable
on Chrome OS lock screen.
The context will be assigned to a script context when it's
associted with a bleseed extension context for an extension/app
that's created with RUNS_ON_LOCK_SCREEN creation flag (introduced
in this cl). The RUNS_ON_LOCK_SCREEN creation flag will be set
when adding lock screen enabled platform app to extension service
for profile used to run lock screen apps. These apps will be used
as handlers for actions (currently only "new_note" exists) on
lock screen.

BUG=715781
==========

to

==========
Introduce lock screen app context to extension features

The context will be used for platform apps that are runnable
on Chrome OS lock screen.
The context will be assigned to a script context when it's
associated with a blessed extension context for a platform app
that's created with RUNS_ON_LOCK_SCREEN creation flag (introduced
in this cl). The RUNS_ON_LOCK_SCREEN flag will be set
when adding lock screen enabled platform app to extension service
for profile used to run lock screen apps. These apps will be used
as handlers for actions (currently only "new_note" exists) on
lock screen.

BUG=715781
==========

[tbarzic, 2017-05-22 23:50:44]

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

[commit-bot: I haz the power, 2017-05-22 23:51:14]

Dry run: CQ is trying da patch.

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

[tbarzic, 2017-05-23 01:17:45]

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

[commit-bot: I haz the power, 2017-05-23 01:18:15]

Dry run: CQ is trying da patch.

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

[tbarzic, 2017-05-23 02:15:32]

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

[commit-bot: I haz the power, 2017-05-23 02:34:15]

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

[commit-bot: I haz the power, 2017-05-23 02:34:16]

Dry run: This issue passed the CQ dry run.

[Devlin, 2017-05-23 16:51:53]

https://codereview.chromium.org/2892403002/diff/40001/extensions/common/exten...
File extensions/common/extension.h (right):

https://codereview.chromium.org/2892403002/diff/40001/extensions/common/exten...
extensions/common/extension.h:165: RUNS_ON_LOCK_SCREEN = 1 << 13,
I'm not sure we want a creation flag for this, but it doesn't seem to be used
anywhere.  Is there a reason you wanted to take this approach?

[tbarzic, 2017-05-23 17:30:24]

https://codereview.chromium.org/2892403002/diff/40001/extensions/common/exten...
File extensions/common/extension.h (right):

https://codereview.chromium.org/2892403002/diff/40001/extensions/common/exten...
extensions/common/extension.h:165: RUNS_ON_LOCK_SCREEN = 1 << 13,
On 2017/05/23 16:51:53, Devlin (catching up) wrote:
> I'm not sure we want a creation flag for this, but it doesn't seem to be used
> anywhere.  Is there a reason you wanted to take this approach?

I'll add code that uses the flag in later cls.

The reason for this approach is that it works with how apps will be set up for
handling actions on lock screen.
The app would be added to a profile not associated with any user (current plan
is to use sign-in profile), and when the app has to be launched, onLaunced
events would be sent to the app in that profile.
Given that the app instances would be designated to handle lock screen requests,
it makes sense  to make that a property of the app instance. And since
'runs_on_lock_screen' value would remain unchanged during the app's lifetime, I
figured to set this value as a creation flag.

basically the meaning of the flag would be "installed into lock-screen profile
with intent to handle actions on lock screen".

[Devlin, 2017-05-25 20:42:02]

https://codereview.chromium.org/2892403002/diff/40001/extensions/common/exten...
File extensions/common/extension.h (right):

https://codereview.chromium.org/2892403002/diff/40001/extensions/common/exten...
extensions/common/extension.h:165: RUNS_ON_LOCK_SCREEN = 1 << 13,
On 2017/05/23 17:30:23, tbarzic wrote:
> On 2017/05/23 16:51:53, Devlin (catching up) wrote:
> > I'm not sure we want a creation flag for this, but it doesn't seem to be
used
> > anywhere.  Is there a reason you wanted to take this approach?
> 
> I'll add code that uses the flag in later cls.
> 
> The reason for this approach is that it works with how apps will be set up for
> handling actions on lock screen.
> The app would be added to a profile not associated with any user (current plan
> is to use sign-in profile), and when the app has to be launched, onLaunced
> events would be sent to the app in that profile.
> Given that the app instances would be designated to handle lock screen
requests,
> it makes sense  to make that a property of the app instance. And since
> 'runs_on_lock_screen' value would remain unchanged during the app's lifetime,
I
> figured to set this value as a creation flag.
> 
> basically the meaning of the flag would be "installed into lock-screen profile
> with intent to handle actions on lock screen".

Sorry for the delay.

I'd prefer we don't have this as a creation flag; it doesn't seem like quite the
right place for it.  My understanding is that the app will still be able to
communicate (in some way) with its non-lock-screen counterpart, and having
different, semi-transitive creation flags seems weird.

Out of curiosity, do we need to store a bit on the extension at all?  If we're
adding this to a lock screen profile, isn't the fact that it's added indicative
of it running on the lock screen?

[tbarzic, 2017-05-25 22:00:18]

On 2017/05/25 20:42:02, Devlin (catching up) wrote:
>
https://codereview.chromium.org/2892403002/diff/40001/extensions/common/exten...
> File extensions/common/extension.h (right):
> 
>
https://codereview.chromium.org/2892403002/diff/40001/extensions/common/exten...
> extensions/common/extension.h:165: RUNS_ON_LOCK_SCREEN = 1 << 13,
> On 2017/05/23 17:30:23, tbarzic wrote:
> > On 2017/05/23 16:51:53, Devlin (catching up) wrote:
> > > I'm not sure we want a creation flag for this, but it doesn't seem to be
> used
> > > anywhere.  Is there a reason you wanted to take this approach?
> > 
> > I'll add code that uses the flag in later cls.
> > 
> > The reason for this approach is that it works with how apps will be set up
for
> > handling actions on lock screen.
> > The app would be added to a profile not associated with any user (current
plan
> > is to use sign-in profile), and when the app has to be launched, onLaunced
> > events would be sent to the app in that profile.
> > Given that the app instances would be designated to handle lock screen
> requests,
> > it makes sense  to make that a property of the app instance. And since
> > 'runs_on_lock_screen' value would remain unchanged during the app's
lifetime,
> I
> > figured to set this value as a creation flag.
> > 
> > basically the meaning of the flag would be "installed into lock-screen
profile
> > with intent to handle actions on lock screen".
> 
> Sorry for the delay.
> 
> I'd prefer we don't have this as a creation flag; it doesn't seem like quite
the
> right place for it.  My understanding is that the app will still be able to
> communicate (in some way) with its non-lock-screen counterpart, and having
> different, semi-transitive creation flags seems weird.
> 

Yeah, there will be a way for the app to "send" data to it's non-lock-screen
counterpart
(where "send" is "create data it's counterpart will be able to import when the
session is unlocked").
Though, those two will still be different app instances, so having different
creation flags does not seem as weird
to me (as lock screen app was created to be able to show app windows on lock
screen).
I can try to pass this information in a different way, but (provided this
information is attached to Extension) it will be very similar
to using a creation flag (just packaged differently, and with more overhead to
pass it to the render process).
Do you have a suggestion for a better way of doing this?

>
> Out of curiosity, do we need to store a bit on the extension at all?  If we're
> adding this to a lock screen profile, isn't the fact that it's added
indicative
> of it running on the lock screen?

Depends on the profile, but it might not be that simple - e.g. the profile might
have other extensions installed
(e.g. ChromeVox comes to mind, given that we'll likely want a11y enabled in the
profile), though, in longer term it might be a good idea to restrict API usage
for these extensions, too. With sign-in profile (which I plan to use, at least
initially), there are also login screen apps. This can be solved by getting more
information about the extension, but we'll still have to derive this information
from the extension (and/or user session state).

The bigger issue is knowing "profile is a lock screen profile" outside of
chrome/, i.e. delegating this piece of information to extensions/common (and
passing it to the render process). I'm not saying it can't be done, but it's
simpler to store this bit with the extension.

[Devlin, 2017-05-27 02:02:23]

On 2017/05/25 22:00:18, tbarzic wrote:
> On 2017/05/25 20:42:02, Devlin (catching up) wrote:
> >
>
https://codereview.chromium.org/2892403002/diff/40001/extensions/common/exten...
> > File extensions/common/extension.h (right):
> > 
> >
>
https://codereview.chromium.org/2892403002/diff/40001/extensions/common/exten...
> > extensions/common/extension.h:165: RUNS_ON_LOCK_SCREEN = 1 << 13,
> > On 2017/05/23 17:30:23, tbarzic wrote:
> > > On 2017/05/23 16:51:53, Devlin (catching up) wrote:
> > > > I'm not sure we want a creation flag for this, but it doesn't seem to be
> > used
> > > > anywhere.  Is there a reason you wanted to take this approach?
> > > 
> > > I'll add code that uses the flag in later cls.
> > > 
> > > The reason for this approach is that it works with how apps will be set up
> for
> > > handling actions on lock screen.
> > > The app would be added to a profile not associated with any user (current
> plan
> > > is to use sign-in profile), and when the app has to be launched, onLaunced
> > > events would be sent to the app in that profile.
> > > Given that the app instances would be designated to handle lock screen
> > requests,
> > > it makes sense  to make that a property of the app instance. And since
> > > 'runs_on_lock_screen' value would remain unchanged during the app's
> lifetime,
> > I
> > > figured to set this value as a creation flag.
> > > 
> > > basically the meaning of the flag would be "installed into lock-screen
> profile
> > > with intent to handle actions on lock screen".
> > 
> > Sorry for the delay.
> > 
> > I'd prefer we don't have this as a creation flag; it doesn't seem like quite
> the
> > right place for it.  My understanding is that the app will still be able to
> > communicate (in some way) with its non-lock-screen counterpart, and having
> > different, semi-transitive creation flags seems weird.
> > 
> 
> Yeah, there will be a way for the app to "send" data to it's non-lock-screen
> counterpart
> (where "send" is "create data it's counterpart will be able to import when the
> session is unlocked").
> Though, those two will still be different app instances, so having different
> creation flags does not seem as weird
> to me (as lock screen app was created to be able to show app windows on lock
> screen).
> I can try to pass this information in a different way, but (provided this
> information is attached to Extension) it will be very similar
> to using a creation flag (just packaged differently, and with more overhead to
> pass it to the render process).
> Do you have a suggestion for a better way of doing this?
> 
> >
> > Out of curiosity, do we need to store a bit on the extension at all?  If
we're
> > adding this to a lock screen profile, isn't the fact that it's added
> indicative
> > of it running on the lock screen?
> 
> Depends on the profile, but it might not be that simple - e.g. the profile
might
> have other extensions installed
> (e.g. ChromeVox comes to mind, given that we'll likely want a11y enabled in
the
> profile), though, in longer term it might be a good idea to restrict API usage
> for these extensions, too. With sign-in profile (which I plan to use, at least
> initially), there are also login screen apps. This can be solved by getting
more
> information about the extension, but we'll still have to derive this
information
> from the extension (and/or user session state).
> 
> The bigger issue is knowing "profile is a lock screen profile" outside of
> chrome/, i.e. delegating this piece of information to extensions/common (and
> passing it to the render process). I'm not saying it can't be done, but it's
> simpler to store this bit with the extension.

An extension shouldn't really need to know details about the profile it's been
added to.  We shouldn't be adding extensions to profiles that don't support
extensions or wouldn't let the extension run; it would just be unneeded
overhead.  From looking at the followup to this patch, it seems like that holds
true in at least one direction - every extension we're adding to the lock screen
profile gets the addition of the creation flag.  As for knowing whether or not a
profile is a lock screen profile outside of chrome, I agree it will be tricky,
but that's part of my point - we shouldn't be bundling the concept of lock
screen profiles in extensions (which are not a chrome concept).  It's sort of an
implicit layering violation.

It seems like we should be able to determine all of this chrome-side before
adding the extension, and the extension system can remain (at least mostly)
oblivious to this, which avoids adding the unnecessary complexity and protects
the layering.

[tbarzic, 2017-05-27 03:27:59]

On 2017/05/27 02:02:23, Devlin (US Holiday May 29) wrote:
> On 2017/05/25 22:00:18, tbarzic wrote:
> > On 2017/05/25 20:42:02, Devlin (catching up) wrote:
> > >
> >
>
https://codereview.chromium.org/2892403002/diff/40001/extensions/common/exten...
> > > File extensions/common/extension.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2892403002/diff/40001/extensions/common/exten...
> > > extensions/common/extension.h:165: RUNS_ON_LOCK_SCREEN = 1 << 13,
> > > On 2017/05/23 17:30:23, tbarzic wrote:
> > > > On 2017/05/23 16:51:53, Devlin (catching up) wrote:
> > > > > I'm not sure we want a creation flag for this, but it doesn't seem to
be
> > > used
> > > > > anywhere.  Is there a reason you wanted to take this approach?
> > > > 
> > > > I'll add code that uses the flag in later cls.
> > > > 
> > > > The reason for this approach is that it works with how apps will be set
up
> > for
> > > > handling actions on lock screen.
> > > > The app would be added to a profile not associated with any user
(current
> > plan
> > > > is to use sign-in profile), and when the app has to be launched,
onLaunced
> > > > events would be sent to the app in that profile.
> > > > Given that the app instances would be designated to handle lock screen
> > > requests,
> > > > it makes sense  to make that a property of the app instance. And since
> > > > 'runs_on_lock_screen' value would remain unchanged during the app's
> > lifetime,
> > > I
> > > > figured to set this value as a creation flag.
> > > > 
> > > > basically the meaning of the flag would be "installed into lock-screen
> > profile
> > > > with intent to handle actions on lock screen".
> > > 
> > > Sorry for the delay.
> > > 
> > > I'd prefer we don't have this as a creation flag; it doesn't seem like
quite
> > the
> > > right place for it.  My understanding is that the app will still be able
to
> > > communicate (in some way) with its non-lock-screen counterpart, and having
> > > different, semi-transitive creation flags seems weird.
> > > 
> > 
> > Yeah, there will be a way for the app to "send" data to it's non-lock-screen
> > counterpart
> > (where "send" is "create data it's counterpart will be able to import when
the
> > session is unlocked").
> > Though, those two will still be different app instances, so having different
> > creation flags does not seem as weird
> > to me (as lock screen app was created to be able to show app windows on lock
> > screen).
> > I can try to pass this information in a different way, but (provided this
> > information is attached to Extension) it will be very similar
> > to using a creation flag (just packaged differently, and with more overhead
to
> > pass it to the render process).
> > Do you have a suggestion for a better way of doing this?
> > 
> > >
> > > Out of curiosity, do we need to store a bit on the extension at all?  If
> we're
> > > adding this to a lock screen profile, isn't the fact that it's added
> > indicative
> > > of it running on the lock screen?
> > 
> > Depends on the profile, but it might not be that simple - e.g. the profile
> might
> > have other extensions installed
> > (e.g. ChromeVox comes to mind, given that we'll likely want a11y enabled in
> the
> > profile), though, in longer term it might be a good idea to restrict API
usage
> > for these extensions, too. With sign-in profile (which I plan to use, at
least
> > initially), there are also login screen apps. This can be solved by getting
> more
> > information about the extension, but we'll still have to derive this
> information
> > from the extension (and/or user session state).
> > 
> > The bigger issue is knowing "profile is a lock screen profile" outside of
> > chrome/, i.e. delegating this piece of information to extensions/common (and
> > passing it to the render process). I'm not saying it can't be done, but it's
> > simpler to store this bit with the extension.
> 
> An extension shouldn't really need to know details about the profile it's been
> added to.  We shouldn't be adding extensions to profiles that don't support
> extensions or wouldn't let the extension run; it would just be unneeded
> overhead.  From looking at the followup to this patch, it seems like that
holds
> true in at least one direction - every extension we're adding to the lock
screen
> profile gets the addition of the creation flag.  As for knowing whether or not
a
> profile is a lock screen profile outside of chrome, I agree it will be tricky,
> but that's part of my point - we shouldn't be bundling the concept of lock
> screen profiles in extensions (which are not a chrome concept).  It's sort of
an
> implicit layering violation.
> 
> It seems like we should be able to determine all of this chrome-side before
> adding the extension, and the extension system can remain (at least mostly)
> oblivious to this, which avoids adding the unnecessary complexity and protects
> the layering.

Yes, I think we agree that we shouldn't be bundling concept of lock screen
profile in extensions; I'd pretty much prefer to avoid that.

But in order to be able to have extension system restrict APIs availble to apps
that can be shown on lock screen, the extension system does have to know an app
can run in the sensitive environment the lock screen is.
That's what I'm actually trying to do - before adding the extension, Chrome
determines the app can run on the lock screen, and marks it as such (the fact
that all of these apps are added to a specifc profile is not that relevant
here). Extension system can then use the information that the extension can run
on the lock screen, and use it to restrict API surface available to that
extension.

Do you have a suggestion on a simpler way on how to pass this information (that
an app can be used in a sensitive environment, in this particular case, on the
lock screen) to extensions system?

[Devlin, 2017-05-30 20:22:30]

On 2017/05/27 03:27:59, tbarzic wrote:
> On 2017/05/27 02:02:23, Devlin (US Holiday May 29) wrote:
> > An extension shouldn't really need to know details about the profile it's
been
> > added to.  We shouldn't be adding extensions to profiles that don't support
> > extensions or wouldn't let the extension run; it would just be unneeded
> > overhead.  From looking at the followup to this patch, it seems like that
> holds
> > true in at least one direction - every extension we're adding to the lock
> screen
> > profile gets the addition of the creation flag.  As for knowing whether or
not
> a
> > profile is a lock screen profile outside of chrome, I agree it will be
tricky,
> > but that's part of my point - we shouldn't be bundling the concept of lock
> > screen profiles in extensions (which are not a chrome concept).  It's sort
of
> an
> > implicit layering violation.
> > 
> > It seems like we should be able to determine all of this chrome-side before
> > adding the extension, and the extension system can remain (at least mostly)
> > oblivious to this, which avoids adding the unnecessary complexity and
protects
> > the layering.
> 
> Yes, I think we agree that we shouldn't be bundling concept of lock screen
> profile in extensions; I'd pretty much prefer to avoid that.
> 
> But in order to be able to have extension system restrict APIs availble to
apps
> that can be shown on lock screen, the extension system does have to know an
app
> can run in the sensitive environment the lock screen is.
> That's what I'm actually trying to do - before adding the extension, Chrome
> determines the app can run on the lock screen, and marks it as such (the fact
> that all of these apps are added to a specifc profile is not that relevant
> here). Extension system can then use the information that the extension can
run
> on the lock screen, and use it to restrict API surface available to that
> extension.
> 
> Do you have a suggestion on a simpler way on how to pass this information
(that
> an app can be used in a sensitive environment, in this particular case, on the
> lock screen) to extensions system?

Restricting the APIs available to that extension should be done as part of the
bindings instantiation, so we *will* need to know whether a given context is the
lock screen (in practice, this will probably end up being an unfortunate
embedder call).  However, this should be independent of the per-extension
knowledge - if an extension is installed in the lock screen profile, it should
be able to run in the lock screen profile (otherwise, we shouldn't install it). 
So I agree we will need some special lock-screen logic, but this should happen
for contexts, not for extensions.  Providing a bit on the extension itself
doesn't help us when we instantiate the context, and I think that's the only
time we'd need that information (since again, my assumption is that we shouldn't
add an extension that can't run to a lock screen profile).

[tbarzic, 2017-05-30 22:51:17]

On 2017/05/30 20:22:30, Devlin wrote:
> On 2017/05/27 03:27:59, tbarzic wrote:
> > On 2017/05/27 02:02:23, Devlin (US Holiday May 29) wrote:
> > > An extension shouldn't really need to know details about the profile it's
> been
> > > added to.  We shouldn't be adding extensions to profiles that don't
support
> > > extensions or wouldn't let the extension run; it would just be unneeded
> > > overhead.  From looking at the followup to this patch, it seems like that
> > holds
> > > true in at least one direction - every extension we're adding to the lock
> > screen
> > > profile gets the addition of the creation flag.  As for knowing whether or
> not
> > a
> > > profile is a lock screen profile outside of chrome, I agree it will be
> tricky,
> > > but that's part of my point - we shouldn't be bundling the concept of lock
> > > screen profiles in extensions (which are not a chrome concept).  It's sort
> of
> > an
> > > implicit layering violation.
> > > 
> > > It seems like we should be able to determine all of this chrome-side
before
> > > adding the extension, and the extension system can remain (at least
mostly)
> > > oblivious to this, which avoids adding the unnecessary complexity and
> protects
> > > the layering.
> > 
> > Yes, I think we agree that we shouldn't be bundling concept of lock screen
> > profile in extensions; I'd pretty much prefer to avoid that.
> > 
> > But in order to be able to have extension system restrict APIs availble to
> apps
> > that can be shown on lock screen, the extension system does have to know an
> app
> > can run in the sensitive environment the lock screen is.
> > That's what I'm actually trying to do - before adding the extension, Chrome
> > determines the app can run on the lock screen, and marks it as such (the
fact
> > that all of these apps are added to a specifc profile is not that relevant
> > here). Extension system can then use the information that the extension can
> run
> > on the lock screen, and use it to restrict API surface available to that
> > extension.
> > 
> > Do you have a suggestion on a simpler way on how to pass this information
> (that
> > an app can be used in a sensitive environment, in this particular case, on
the
> > lock screen) to extensions system?
> 
> Restricting the APIs available to that extension should be done as part of the
> bindings instantiation, so we *will* need to know whether a given context is
the
> lock screen (in practice, this will probably end up being an unfortunate
> embedder call).  However, this should be independent of the per-extension
> knowledge - if an extension is installed in the lock screen profile, it should
> be able to run in the lock screen profile (otherwise, we shouldn't install
it). 
> So I agree we will need some special lock-screen logic, but this should happen
> for contexts, not for extensions.  Providing a bit on the extension itself
> doesn't help us when we instantiate the context, and I think that's the only
> time we'd need that information (since again, my assumption is that we
shouldn't
> add an extension that can't run to a lock screen profile).


I guess I had a different idea of lock screen app context - "context of an app
that is allowed to show windows on lock screen", rather than "extension that is
running in lock screen enabled profile (i.e. sign-in profile)".
And latter does not necessarily imply the former (and it makes me a little
concerned about breaking some component extension we might want to have in the
sign-in profile).

But OK, let me update the CL not to derive lock screen context from
per-extension flag.

[tbarzic, 2017-05-31 01:56:08]

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

[commit-bot: I haz the power, 2017-05-31 01:56: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, 2017-05-31 02:00:09]

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

[commit-bot: I haz the power, 2017-05-31 02:00:10]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[tbarzic, 2017-05-31 02:52:14]

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

[commit-bot: I haz the power, 2017-05-31 02:53:05]

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-05-31 04:25:12]

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

[commit-bot: I haz the power, 2017-05-31 04:25:13]

Dry run: This issue passed the CQ dry run.

[tbarzic, 2017-05-31 17:10:12]

Description was changed from

==========
Introduce lock screen app context to extension features

The context will be used for platform apps that are runnable
on Chrome OS lock screen.
The context will be assigned to a script context when it's
associated with a blessed extension context for a platform app
that's created with RUNS_ON_LOCK_SCREEN creation flag (introduced
in this cl). The RUNS_ON_LOCK_SCREEN flag will be set
when adding lock screen enabled platform app to extension service
for profile used to run lock screen apps. These apps will be used
as handlers for actions (currently only "new_note" exists) on
lock screen.

BUG=715781
==========

to

==========
Introduce lock screen app context to extension features

The context will be used for platform apps that are runnable
on Chrome OS lock screen - the context will be derived from the
containing browser process. Apps running in sign-in profile after
the user was logged in will run in 'lock screen' script context.
Because sign-in profile is Chrome concept, unknown to extensions
layer, the information will have to be relayed to extensions system.
The script context is determined in two places:
* ScriptContextSet, which determines script context used in the render 
  process. The script context set will be flagged as lock screen 
  context during renderer initialization - it will be sent from the 
  browser process in SetSessionInfo IPC message.
* ProcessMap, used in browser process. ProcessMaps are created per 
  browser context - process maps belonging to sign-in profile will be
  marked as lock screen context process map (as before that point, the 
  sign-in profile will be used for login)

BUG=715781
==========

[tbarzic, 2017-05-31 17:32:26]

Updated the cl to push the 'lock screen context' bit from Chrome to extensions
layer based on current profile and user session state.
Hope it looks more acceptable :)

[Devlin, 2017-06-01 22:17:58]

Nice, just a few last concerns but otherwise this looks pretty good.

https://codereview.chromium.org/2892403002/diff/120001/chrome/browser/extensi...
File chrome/browser/extensions/chrome_extensions_browser_client.cc (right):

https://codereview.chromium.org/2892403002/diff/120001/chrome/browser/extensi...
chrome/browser/extensions/chrome_extensions_browser_client.cc:442: bool
ChromeExtensionsBrowserClient::IsLockScreenContext(
Is there a race condition here?  The comment in extension service implies that
we don't know if a profile is a lock screen profile at initialization; are we
sure that this will only be called after the appropriate values are set?

https://codereview.chromium.org/2892403002/diff/120001/chrome/common/extensio...
File chrome/common/extensions/api/_features.md (right):

https://codereview.chromium.org/2892403002/diff/120001/chrome/common/extensio...
chrome/common/extensions/api/_features.md:209: `lock_screen_extension`,
`web_page`, `webui`, and `unblessed_extension`.
will there be any other type of context on the lock screen?  I'm wondering if
this is conceptually most similar to context or something like session type.

https://codereview.chromium.org/2892403002/diff/120001/extensions/browser/pro...
File extensions/browser/process_map_unittest.cc (right):

https://codereview.chromium.org/2892403002/diff/120001/extensions/browser/pro...
extensions/browser/process_map_unittest.cc:27: if (type ==
TypeToCreate::kExtension) {
prefer a switch

https://codereview.chromium.org/2892403002/diff/120001/extensions/renderer/sc...
File extensions/renderer/script_context_set.h (right):

https://codereview.chromium.org/2892403002/diff/120001/extensions/renderer/sc...
extensions/renderer/script_context_set.h:109: void set_lock_screen_context(bool
lock_screen_context) {
nit: prefer set_*is*_lock_screen_context (to differentiate between a bool and,
e.g., a browser context)

[tbarzic, 2017-06-02 00:49:05]

https://codereview.chromium.org/2892403002/diff/120001/chrome/browser/extensi...
File chrome/browser/extensions/chrome_extensions_browser_client.cc (right):

https://codereview.chromium.org/2892403002/diff/120001/chrome/browser/extensi...
chrome/browser/extensions/chrome_extensions_browser_client.cc:442: bool
ChromeExtensionsBrowserClient::IsLockScreenContext(
On 2017/06/01 22:17:58, Devlin wrote:
> Is there a race condition here?  The comment in extension service implies that
> we don't know if a profile is a lock screen profile at initialization; are we
> sure that this will only be called after the appropriate values are set?

I think it should be fine - renderers for apps launched in sign-in profile after
the sign-in profile becomes "lock-screen" profile (i.e. the ones that should
have lock screen context) should be created after session starts (there might be
some apps installed in the profile from before, but it is not expected for them
to run after user session is created). Though, checking session state would
probably be more in line with what's done in extension_service.

I'm not too happy with the fact that sign-in profile essentially goes from being
'login context' to being 'lock screen' context (seems a little too fragile for
my taste), so I intend to look into introducing new profile type designated for
lock screen apps before the feature launches (but don't want to block other
pieces of the feature on it).

https://codereview.chromium.org/2892403002/diff/120001/chrome/common/extensio...
File chrome/common/extensions/api/_features.md (right):

https://codereview.chromium.org/2892403002/diff/120001/chrome/common/extensio...
chrome/common/extensions/api/_features.md:209: `lock_screen_extension`,
`web_page`, `webui`, and `unblessed_extension`.
On 2017/06/01 22:17:58, Devlin wrote:
> will there be any other type of context on the lock screen?  I'm wondering if
> this is conceptually most similar to context or something like session type.

I was thinking about adding it to session type, but I think it's closer to
context (as lock screen is essentially a state of regular user session).

I don't expect any other context types on lock screen (at least not something
that would be relevant for extension features) - if that expectation does not
hold, I'd consider adding a separate "setting" for this; maybe session_state:
{LOCKED, ACTIVE, NOT_STARTED}.

https://codereview.chromium.org/2892403002/diff/120001/extensions/browser/pro...
File extensions/browser/process_map_unittest.cc (right):

https://codereview.chromium.org/2892403002/diff/120001/extensions/browser/pro...
extensions/browser/process_map_unittest.cc:27: if (type ==
TypeToCreate::kExtension) {
On 2017/06/01 22:17:58, Devlin wrote:
> prefer a switch

Done.

https://codereview.chromium.org/2892403002/diff/120001/extensions/renderer/sc...
File extensions/renderer/script_context_set.h (right):

https://codereview.chromium.org/2892403002/diff/120001/extensions/renderer/sc...
extensions/renderer/script_context_set.h:109: void set_lock_screen_context(bool
lock_screen_context) {
On 2017/06/01 22:17:58, Devlin wrote:
> nit: prefer set_*is*_lock_screen_context (to differentiate between a bool and,
> e.g., a browser context)

Done.

[Devlin, 2017-06-03 02:19:15]

lgtm.  I still have some questions, but I don't think they need to block this.

https://codereview.chromium.org/2892403002/diff/120001/chrome/browser/extensi...
File chrome/browser/extensions/chrome_extensions_browser_client.cc (right):

https://codereview.chromium.org/2892403002/diff/120001/chrome/browser/extensi...
chrome/browser/extensions/chrome_extensions_browser_client.cc:442: bool
ChromeExtensionsBrowserClient::IsLockScreenContext(
On 2017/06/02 00:49:05, tbarzic wrote:
> On 2017/06/01 22:17:58, Devlin wrote:
> > Is there a race condition here?  The comment in extension service implies
that
> > we don't know if a profile is a lock screen profile at initialization; are
we
> > sure that this will only be called after the appropriate values are set?
> 
> I think it should be fine - renderers for apps launched in sign-in profile
after
> the sign-in profile becomes "lock-screen" profile (i.e. the ones that should
> have lock screen context) should be created after session starts (there might
be
> some apps installed in the profile from before, but it is not expected for
them
> to run after user session is created). Though, checking session state would
> probably be more in line with what's done in extension_service.
> 
> I'm not too happy with the fact that sign-in profile essentially goes from
being
> 'login context' to being 'lock screen' context (seems a little too fragile for
> my taste), so I intend to look into introducing new profile type designated
for
> lock screen apps before the feature launches (but don't want to block other
> pieces of the feature on it).

+1 to having this be a new state in the future.

https://codereview.chromium.org/2892403002/diff/140001/chrome/common/extensio...
File chrome/common/extensions/api/_features.md (right):

https://codereview.chromium.org/2892403002/diff/140001/chrome/common/extensio...
chrome/common/extensions/api/_features.md:209: `lock_screen_extension`,
`web_page`, `webui`, and `unblessed_extension`.
(assuming this is correct) It might be worth adding a little extra detail about
lock_screen_extension; in particular that any context on the lock screen is
classified as lock_screen_extension.

https://codereview.chromium.org/2892403002/diff/140001/extensions/browser/pro...
File extensions/browser/process_map.cc (right):

https://codereview.chromium.org/2892403002/diff/140001/extensions/browser/pro...
extensions/browser/process_map.cc:143: return is_lock_screen_context_ ?
Feature::LOCK_SCREEN_EXTENSION_CONTEXT
If we expect no other lock-screen-type contexts, maybe it'd make sense to put
this at the top?

if (is_lock_screen_context_)
  return FEATURE::LOCK_SCREEN_CONTEXT;

if (content::ChildProcess....)

return Feature::BLESSED_EXTENSION_CONTEXT;

Because we *are* determining that any lock screen context is considered a lock
screen extension context, right?  Or are we anticipating having multiple
processes on the lock screen, some of which are "blessed" lock screen contexts,
and others that would be "web-page like" contexts?

https://codereview.chromium.org/2892403002/diff/140001/extensions/browser/tes...
File extensions/browser/test_extensions_browser_client.cc (right):

https://codereview.chromium.org/2892403002/diff/140001/extensions/browser/tes...
extensions/browser/test_extensions_browser_client.cc:229: }
nit: \n

https://codereview.chromium.org/2892403002/diff/140001/extensions/renderer/sc...
File extensions/renderer/script_context_set.cc (right):

https://codereview.chromium.org/2892403002/diff/140001/extensions/renderer/sc...
extensions/renderer/script_context_set.cc:207: return is_lock_screen_context_ ?
Feature::LOCK_SCREEN_EXTENSION_CONTEXT
similar question to process map

[tbarzic, 2017-06-05 19:35:40]

https://codereview.chromium.org/2892403002/diff/140001/chrome/common/extensio...
File chrome/common/extensions/api/_features.md (right):

https://codereview.chromium.org/2892403002/diff/140001/chrome/common/extensio...
chrome/common/extensions/api/_features.md:209: `lock_screen_extension`,
`web_page`, `webui`, and `unblessed_extension`.
On 2017/06/03 02:19:14, Devlin wrote:
> (assuming this is correct) It might be worth adding a little extra detail
about
> lock_screen_extension; in particular that any context on the lock screen is
> classified as lock_screen_extension.

added more detail about lock screen expectations.

https://codereview.chromium.org/2892403002/diff/140001/extensions/browser/pro...
File extensions/browser/process_map.cc (right):

https://codereview.chromium.org/2892403002/diff/140001/extensions/browser/pro...
extensions/browser/process_map.cc:143: return is_lock_screen_context_ ?
Feature::LOCK_SCREEN_EXTENSION_CONTEXT
On 2017/06/03 02:19:14, Devlin wrote:
> If we expect no other lock-screen-type contexts, maybe it'd make sense to put
> this at the top?
> 
> if (is_lock_screen_context_)
>   return FEATURE::LOCK_SCREEN_CONTEXT;
> 
> if (content::ChildProcess....)
> 
> return Feature::BLESSED_EXTENSION_CONTEXT;
> 
> Because we *are* determining that any lock screen context is considered a lock
> screen extension context, right?  Or are we anticipating having multiple
> processes on the lock screen, some of which are "blessed" lock screen
contexts,
> and others that would be "web-page like" contexts?

At the moment, we could have web-page like contexts, in particular lock screen
UI is currently implemented as webui (though, that will probably change in near
future).

For content scripts/hosted apps, these should never be active for lock_screen
context, so it might be better to have a CHECK(!is_lock_screen_context_) or
return unspecified context instead of forcing LOCK_SCREEN_EXTENSION_CONTEXT.
WDYT?

https://codereview.chromium.org/2892403002/diff/140001/extensions/browser/tes...
File extensions/browser/test_extensions_browser_client.cc (right):

https://codereview.chromium.org/2892403002/diff/140001/extensions/browser/tes...
extensions/browser/test_extensions_browser_client.cc:229: }
On 2017/06/03 02:19:15, Devlin wrote:
> nit: \n

Done.

https://codereview.chromium.org/2892403002/diff/140001/extensions/renderer/sc...
File extensions/renderer/script_context_set.cc (right):

https://codereview.chromium.org/2892403002/diff/140001/extensions/renderer/sc...
extensions/renderer/script_context_set.cc:207: return is_lock_screen_context_ ?
Feature::LOCK_SCREEN_EXTENSION_CONTEXT
On 2017/06/03 02:19:15, Devlin wrote:
> similar question to process map

pretty much the same answer :)

[tbarzic, 2017-06-08 18:06:15]

tbarzic@chromium.org changed reviewers:
+ meacer@chromium.org

[tbarzic, 2017-06-08 18:06:16]

+meacer for extensions/common/extension_messages.h

[tbarzic, 2017-06-09 21:10:11]

tbarzic@chromium.org changed reviewers:
+ isherman@chromium.org

[tbarzic, 2017-06-09 21:10:12]

+isherman for histograms.xml

[Ilya Sherman, 2017-06-09 21:11:38]

Metrics LGTM

https://codereview.chromium.org/2892403002/diff/160001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2892403002/diff/160001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:19052: +    context.
nit: Mebbe mention "lock screen" somewhere in this description?

[tbarzic, 2017-06-09 21:26:34]

https://codereview.chromium.org/2892403002/diff/160001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2892403002/diff/160001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:19052: +    context.
On 2017/06/09 21:11:38, Ilya Sherman wrote:
> nit: Mebbe mention "lock screen" somewhere in this description?

Done.

[meacer, 2017-06-09 21:32:34]

extensions/common/extension_messages.h Lgtm

https://codereview.chromium.org/2892403002/diff/180001/extensions/browser/ext...
File extensions/browser/extensions_browser_client.h (right):

https://codereview.chromium.org/2892403002/diff/180001/extensions/browser/ext...
extensions/browser/extensions_browser_client.h:266: // Whether the browser
context is associted with Chrome OS lock screen.
associted -> associated

[tbarzic, 2017-06-09 21:47:35]

https://codereview.chromium.org/2892403002/diff/180001/extensions/browser/ext...
File extensions/browser/extensions_browser_client.h (right):

https://codereview.chromium.org/2892403002/diff/180001/extensions/browser/ext...
extensions/browser/extensions_browser_client.h:266: // Whether the browser
context is associted with Chrome OS lock screen.
On 2017/06/09 21:32:34, meacer wrote:
> associted -> associated

Done.

[tbarzic, 2017-06-09 21:48:04]

The CQ bit was checked by tbarzic@chromium.org

[tbarzic, 2017-06-09 21:48:05]

The patchset sent to the CQ was uploaded after l-g-t-m from
rdevlin.cronin@chromium.org, isherman@chromium.org, meacer@chromium.org
Link to the patchset: https://codereview.chromium.org/2892403002/#ps200001
(title: ".")

[commit-bot: I haz the power, 2017-06-09 21:48:41]

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-09 22:08:03]

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

[commit-bot: I haz the power, 2017-06-09 22:08:05]

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

[tbarzic, 2017-06-09 23:21:56]

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

[commit-bot: I haz the power, 2017-06-09 23:22:27]

Dry run: CQ is trying da patch.

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

[tbarzic, 2017-06-09 23:41:59]

The CQ bit was unchecked by tbarzic@chromium.org

[tbarzic, 2017-06-09 23:41:59]

The CQ bit was checked by tbarzic@chromium.org

[tbarzic, 2017-06-09 23:42:00]

The patchset sent to the CQ was uploaded after l-g-t-m from
rdevlin.cronin@chromium.org, isherman@chromium.org, meacer@chromium.org
Link to the patchset: https://codereview.chromium.org/2892403002/#ps240001
(title: ".")

[commit-bot: I haz the power, 2017-06-09 23:42:34]

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-10 01:46:49]

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

[commit-bot: I haz the power, 2017-06-10 01:46:51]

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

[tbarzic, 2017-06-10 02:20:40]

The CQ bit was checked by tbarzic@chromium.org

[tbarzic, 2017-06-10 02:20:42]

The patchset sent to the CQ was uploaded after l-g-t-m from
rdevlin.cronin@chromium.org, isherman@chromium.org, meacer@chromium.org
Link to the patchset: https://codereview.chromium.org/2892403002/#ps280001
(title: ".")

[commit-bot: I haz the power, 2017-06-10 02:20:57]

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-10 03:25:54]

CQ is committing da patch.

Bot data: {"patchset_id": 280001, "attempt_start_ts": 1497061240913780,
"parent_rev": "a5248677d5d7973aa85d210e3f0ba49b483c6980", "commit_rev":
"8e89b0b1a1bd64d016a24a182a715aeedb712469"}

[commit-bot: I haz the power, 2017-06-10 03:26:05]

Description was changed from

==========
Introduce lock screen app context to extension features

The context will be used for platform apps that are runnable
on Chrome OS lock screen - the context will be derived from the
containing browser process. Apps running in sign-in profile after
the user was logged in will run in 'lock screen' script context.
Because sign-in profile is Chrome concept, unknown to extensions
layer, the information will have to be relayed to extensions system.
The script context is determined in two places:
* ScriptContextSet, which determines script context used in the render 
  process. The script context set will be flagged as lock screen 
  context during renderer initialization - it will be sent from the 
  browser process in SetSessionInfo IPC message.
* ProcessMap, used in browser process. ProcessMaps are created per 
  browser context - process maps belonging to sign-in profile will be
  marked as lock screen context process map (as before that point, the 
  sign-in profile will be used for login)

BUG=715781
==========

to

==========
Introduce lock screen app context to extension features

The context will be used for platform apps that are runnable
on Chrome OS lock screen - the context will be derived from the
containing browser process. Apps running in sign-in profile after
the user was logged in will run in 'lock screen' script context.
Because sign-in profile is Chrome concept, unknown to extensions
layer, the information will have to be relayed to extensions system.
The script context is determined in two places:
* ScriptContextSet, which determines script context used in the render 
  process. The script context set will be flagged as lock screen 
  context during renderer initialization - it will be sent from the 
  browser process in SetSessionInfo IPC message.
* ProcessMap, used in browser process. ProcessMaps are created per 
  browser context - process maps belonging to sign-in profile will be
  marked as lock screen context process map (as before that point, the 
  sign-in profile will be used for login)

BUG=715781

Review-Url: https://codereview.chromium.org/2892403002
Cr-Commit-Position: refs/heads/master@{#478506}
Committed:
https://chromium.googlesource.com/chromium/src/+/8e89b0b1a1bd64d016a24a182a71...
==========

[commit-bot: I haz the power, 2017-06-10 03:26:07]

Committed patchset #15 (id:280001) as
https://chromium.googlesource.com/chromium/src/+/8e89b0b1a1bd64d016a24a182a71...