mojo: Sketch a profile application.

mojo: Sketch a profile application.

This application is bound to an individual profile data dir. While it
isn't yet sandboxed to that location, that's our long term intention
with this design. This application colocates the Profile service and the
LevelDB service so that these will be in the same process.

The way we register a Browser Context to a user id works for now, but
will need to be redone once we start spawning processes for the Profile.

BUG=585587
Committed: https://crrev.com/e69130f5b1a31d11badc7e034252038dc03b8ec6
Cr-Commit-Position: refs/heads/master@{#378620}

[Elliot Glaysher, 2016-02-27 00:01:48]

erg@chromium.org changed reviewers:
+ jam@chromium.org

[Elliot Glaysher, 2016-02-27 00:01:48]

For your bus ride home.

(Will modify https://codereview.chromium.org/1737933002/ onto this if you think
this approach is good.)

[jam, 2016-02-27 01:34:44]

jam@chromium.org changed reviewers:
+ ben@chromium.org

[jam, 2016-02-27 01:34:45]

+Ben as well, he's thought about this more than me

https://codereview.chromium.org/1741953002/diff/80001/chrome/browser/profiles...
File chrome/browser/profiles/profile_impl.cc (right):

https://codereview.chromium.org/1741953002/diff/80001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl.cc:463:
content::BrowserContext::InitializeProfileApplication(this);
need to do this for other BrowserContexts, and add asserts to BrowserContext
that it's initialized.

i.e. TestBrowserContext, ShellBrowserContext, MockBrowserContext,
BlimpBrowserContext

https://codereview.chromium.org/1741953002/diff/80001/components/profile_serv...
File components/profile_service/profile_app.h (right):

https://codereview.chromium.org/1741953002/diff/80001/components/profile_serv...
components/profile_service/profile_app.h:26: // This Application serves
ProfileService, and serves LevelDBService since most
I need to understand this some more. why should the profile app have to know
about leveldb? naively, it should have a GetPath directory that returns the
profile directory, and then that could be used to initialize leveldb instances?

to be clear: some leveldb instances will eventually run in the profile process
(i.e. for LocalStorage). Others might run in the renderer, i.e. for IndexedDB.

https://codereview.chromium.org/1741953002/diff/80001/components/profile_serv...
components/profile_service/profile_app.h:44: // itself and call us to register
the mapping from |user_id| to
nit: seems like a comment layering violation for this component to be talking
about chrome process and BrowserContext?

https://codereview.chromium.org/1741953002/diff/80001/components/profile_serv...
File components/profile_service/profile_service_impl.cc (right):

https://codereview.chromium.org/1741953002/diff/80001/components/profile_serv...
components/profile_service/profile_service_impl.cc:33: std::string
sanitized_origin = BuildSanitizedOrigin(
it wasn't clear to me that GetDirectory returns a per-requester-app directory,
instead of the profile directory. as we convert code to this new architecture,
it seems like pieces would need the root profile directory, at the very least
for migration?

https://codereview.chromium.org/1741953002/diff/80001/components/profile_serv...
File components/profile_service/public/interfaces/profile.mojom (right):

https://codereview.chromium.org/1741953002/diff/80001/components/profile_serv...
components/profile_service/public/interfaces/profile.mojom:9: // An
encapsulation around the per-profile storage to disk. A Profile is
nit: we're going to add stuff to this later that's not just disk, so you can
skip the "storage to disk".

https://codereview.chromium.org/1741953002/diff/80001/content/browser/browser...
File content/browser/browser_context.cc (right):

https://codereview.chromium.org/1741953002/diff/80001/content/browser/browser...
content/browser/browser_context.cc:48:
base::LazyInstance<std::map<BrowserContext*, uint32_t> > g_context_to_user_id =
nit: map is overkill when we know the median value is 1. why not just vector of
std::pair ?

https://codereview.chromium.org/1741953002/diff/80001/content/browser/browser...
content/browser/browser_context.cc:333: void
BrowserContext::InitializeProfileApplication(
nit: src/content has avoided "Profile". i do agree that with a mojom, we should
use profile as the terminology instead of this confusing browsercontext vs
profile split we had between content and chrome. but having a Profile method in
BrowserContext is confusing. can you just call this Initialize?

https://codereview.chromium.org/1741953002/diff/80001/content/browser/browser...
content/browser/browser_context.cc:335: // TODO(erg): The userids should be 64
bit numbers, not 32 bit numbers. Our
I'm not following the comment. What exaclty is stopping you from using 64 bit
now?

https://codereview.chromium.org/1741953002/diff/80001/content/public/browser/...
File content/public/browser/browser_context.h (right):

https://codereview.chromium.org/1741953002/diff/80001/content/public/browser/...
content/public/browser/browser_context.h:131: static void
InitializeProfileApplication(BrowserContext* browser_context);
nit: add comments for both of these

[Elliot Glaysher, 2016-02-29 23:04:13]

ptal; changed the implementation of ProfileService after we discussed this IRL
so that things aren't (currently) dependent on application identity working.

https://codereview.chromium.org/1741953002/diff/80001/components/profile_serv...
File components/profile_service/profile_app.h (right):

https://codereview.chromium.org/1741953002/diff/80001/components/profile_serv...
components/profile_service/profile_app.h:26: // This Application serves
ProfileService, and serves LevelDBService since most
On 2016/02/27 01:34:45, jam wrote:
> naively, it should have a GetPath directory that returns the
> profile directory, and then that could be used to initialize leveldb
instances?

That is in fact how it works. The Profile service has its own interface and
doesn't know about the leveldb service, and vice versa.

> I need to understand this some more. why should the profile app have to know
> about leveldb? 

This is a mere packaging decision. The profile app is not the profile service.
(I think it's really unfortunate that we've ended up with the distinction
between apps and services conflated.)

Until we get something like the core_services packaging, this is how you'd
colocate two services into the same process. (Or do you know of a better way to
colocate services in the same process for now?)

> to be clear: some leveldb instances will eventually run in the profile process
> (i.e. for LocalStorage). Others might run in the renderer, i.e. for IndexedDB.

Which is why it's separated into a service which can be inserted into any
application.

https://codereview.chromium.org/1741953002/diff/80001/components/profile_serv...
components/profile_service/profile_app.h:44: // itself and call us to register
the mapping from |user_id| to
On 2016/02/27 01:34:45, jam wrote:
> nit: seems like a comment layering violation for this component to be talking
> about chrome process and BrowserContext?

Correct, it is a layering violation. Which is why it's documented, along with a
todo about what needs to be done to clean it up.

https://codereview.chromium.org/1741953002/diff/80001/components/profile_serv...
File components/profile_service/profile_service_impl.cc (right):

https://codereview.chromium.org/1741953002/diff/80001/components/profile_serv...
components/profile_service/profile_service_impl.cc:33: std::string
sanitized_origin = BuildSanitizedOrigin(
On 2016/02/27 01:34:45, jam wrote:
> it wasn't clear to me that GetDirectory returns a per-requester-app directory,
> instead of the profile directory. as we convert code to this new architecture,
> it seems like pieces would need the root profile directory, at the very least
> for migration?

My assumption was that the conversion code would live outside the mojoified
world. As the current localstorage stuff is backed by sqlite, and we wouldn't be
able to read that data from inside the sandbox anymore.

Am I incorrect here?

https://codereview.chromium.org/1741953002/diff/80001/content/browser/browser...
File content/browser/browser_context.cc (right):

https://codereview.chromium.org/1741953002/diff/80001/content/browser/browser...
content/browser/browser_context.cc:335: // TODO(erg): The userids should be 64
bit numbers, not 32 bit numbers. Our
On 2016/02/27 01:34:45, jam wrote:
> I'm not following the comment. What exaclty is stopping you from using 64 bit
> now?

Rephrased the comment to point out that mojo userids are unsigned 32 bit
numbers.

[jam, 2016-03-01 21:41:43]

lgtm with one comment per in person

[Elliot Glaysher, 2016-03-01 21:50:21]

The CQ bit was checked by erg@chromium.org

[Elliot Glaysher, 2016-03-01 21:50:21]

The patchset sent to the CQ was uploaded after l-g-t-m from jam@chromium.org
Link to the patchset: https://codereview.chromium.org/1741953002/#ps380001
(title: "Don't add a boolean to BrowserContext; allocate an object instead.")

[commit-bot: I haz the power, 2016-03-01 21:50:51]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1741953002/380001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1741953002/380001

[Ben Goodger (Google), 2016-03-01 21:56:07]

https://codereview.chromium.org/1741953002/diff/360001/components/profile_ser...
File components/profile_service/BUILD.gn (right):

https://codereview.chromium.org/1741953002/diff/360001/components/profile_ser...
components/profile_service/BUILD.gn:8: source_set("lib") {
jam, this is an application I think of as ultimately being a top level "system
service".

WDYT about putting such services in mojo/services?

e.g. in this case, mojo/services/user

https://codereview.chromium.org/1741953002/diff/360001/content/public/browser...
File content/public/browser/browser_context.h (right):

https://codereview.chromium.org/1741953002/diff/360001/content/public/browser...
content/public/browser/browser_context.h:133: static void
Initialize(BrowserContext* browser_context,
is there a reason why these methods tend to be static rather than on this
particular browsercontext?

[commit-bot: I haz the power, 2016-03-01 22:12:39]

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

[commit-bot: I haz the power, 2016-03-01 22:12:41]

Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Elliot Glaysher, 2016-03-01 22:15:49]

The CQ bit was checked by erg@chromium.org

[commit-bot: I haz the power, 2016-03-01 22:16:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1741953002/380001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1741953002/380001

[commit-bot: I haz the power, 2016-03-02 00:13:36]

Committed patchset #20 (id:380001)

[commit-bot: I haz the power, 2016-03-02 00:15:20]

Description was changed from

==========
mojo: Sketch a profile application.

This application is bound to an individual profile data dir. While it
isn't yet sandboxed to that location, that's our long term intention
with this design. This application colocates the Profile service and the
LevelDB service so that these will be in the same process.

The way we register a Browser Context to a user id works for now, but
will need to be redone once we start spawning processes for the Profile.

BUG=585587
==========

to

==========
mojo: Sketch a profile application.

This application is bound to an individual profile data dir. While it
isn't yet sandboxed to that location, that's our long term intention
with this design. This application colocates the Profile service and the
LevelDB service so that these will be in the same process.

The way we register a Browser Context to a user id works for now, but
will need to be redone once we start spawning processes for the Profile.

BUG=585587

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

[commit-bot: I haz the power, 2016-03-02 00:15:21]

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

[jam, 2016-03-02 17:50:38]

https://codereview.chromium.org/1741953002/diff/360001/components/profile_ser...
File components/profile_service/BUILD.gn (right):

https://codereview.chromium.org/1741953002/diff/360001/components/profile_ser...
components/profile_service/BUILD.gn:8: source_set("lib") {
On 2016/03/01 21:56:06, Ben Goodger (Google) wrote:
> jam, this is an application I think of as ultimately being a top level "system
> service".
> 
> WDYT about putting such services in mojo/services?
> 
> e.g. in this case, mojo/services/user

agree

https://codereview.chromium.org/1741953002/diff/360001/content/public/browser...
File content/public/browser/browser_context.h (right):

https://codereview.chromium.org/1741953002/diff/360001/content/public/browser...
content/public/browser/browser_context.h:133: static void
Initialize(BrowserContext* browser_context,
On 2016/03/01 21:56:06, Ben Goodger (Google) wrote:
> is there a reason why these methods tend to be static rather than on this
> particular browsercontext?

that's the (admittedly awkward) pattern used here. browsercontext methods
implemented by the embedder are virtual methods. methods that interact with it
and are implemented in content are static.

this should be split into two interfaces.. but probably a lot of this will go
away once the mojo refactorings are gone