Add BlimpClientContext and factory with real and dummy implementation

Add BlimpClientContext and factory with real and dummy implementation

The BlimpClientContext is the core class which wires up all of the blimp
functionality. When it's created, it will get all its dependencies from
the embedder. It is currently in charge of creating BlimpContents, but
will also own things like the BlimpClientSession. It is part of the
public //blimp/client/public API. It is a KeyedService and is typically
tied to a Profile.

The BlimpClientContextFactory lives in //chrome to be able to access
Profile during construction, since //blimp/client currently does not
depend on //content where BrowserContext lives, and there are upcoming
dependencies that will be implemented by classes in //chrome.

The BlimpClientContext has two implementations in
//blimp/client/core, an actual one and a dummy implementation. The
dummy implementation is to be used when we do not want the blimp
code to be used, and the core implementation is the real blimp
implementation. Which one of these are in use is controlled by the
GN argument enable_blimp which defaults to false.

The BlimpClientContext has a public static create-method that is not
implemented in //blimp/client/public, but instead it is implemented by
//blimp/client/core by both the actual and the dummy implementation,
so the right method will be chosen at link-time. This means that a
target can only link with either the actual or dummy implementation.
Everything in //blimp/client/core is only visible to //blimp/client/*
and embedders should only ever depend on //blimp/client/public, which
will bring in the correct implementation. This works the same for both
C++ and Java, so when using the BlimpClientContextFactory (either in
Java or C++), the right implementation (actual or dummy) will always
be chosen correctly based on the link-time selection of the
Create-method.

The BlimpClientContext has a delegate that it can call out to whenever
it needs specific functionality from the embedder. As an example, this
CL adds a helper for attaching a Profile to every created
BlimpContents to make have similar functionality as what is built into
WebContents so one can get a Profile and thereby services by only
having access to a BlimpContents.

BUG=611094
TBR=erg@chromium.org

Committed: https://crrev.com/9aa0cdee23229ca95a3bea79abcc584a395d016e
Cr-Commit-Position: refs/heads/master@{#405860}

[nyquist, 2016-07-12 19:45:39]

Description was changed from

==========
Add BlimpClientContext

BUG=None
==========

to

==========
Add BlimpClientContext and factory

The BlimpClientContext is the core class which wires up all of the blimp
functionality. When it's created, it will get all its dependencies from
the embedder. It is currently in charge of creating BlimpContents, but
will also own things like the BlimpClientSession. It is part of the
public //blimp/client/public API. It is a KeyedService and is typically
tied to a profile.

The BlimpClientContextFactory lives in //chrome to be able to access
Profile during construction, since //blimp/client currently does not
depend on //content, and there are upcoming dependencies that will
probably be implemented by classes in //chrome.

BUG=None
==========

[nyquist, 2016-07-12 21:45:50]

Description was changed from

==========
Add BlimpClientContext and factory

The BlimpClientContext is the core class which wires up all of the blimp
functionality. When it's created, it will get all its dependencies from
the embedder. It is currently in charge of creating BlimpContents, but
will also own things like the BlimpClientSession. It is part of the
public //blimp/client/public API. It is a KeyedService and is typically
tied to a profile.

The BlimpClientContextFactory lives in //chrome to be able to access
Profile during construction, since //blimp/client currently does not
depend on //content, and there are upcoming dependencies that will
probably be implemented by classes in //chrome.

BUG=None
==========

to

==========
Add BlimpClientContext and factory with real and dummy implementation

The BlimpClientContext is the core class which wires up all of the blimp
functionality. When it's created, it will get all its dependencies from
the embedder. It is currently in charge of creating BlimpContents, but
will also own things like the BlimpClientSession. It is part of the
public //blimp/client/public API. It is a KeyedService and is typically
tied to a profile.

The BlimpClientContextFactory lives in //chrome to be able to access
Profile during construction, since //blimp/client currently does not
depend on //content, and there are upcoming dependencies that will
probably be implemented by classes in //chrome.

The BlimpClientContext has two implementations, one in
//blimp/client/core and one in //blimp/client/dummy. The dummy
implementation is to be used when we do not want the blimp
code to be used, and the core implementation is the real blimp
implementation.

The BlimpClientContext has a public static create-method that is not
implemented in //blimp/client/public, but instead it is implemented by
//blimp/client/core and //blimp/client/dummy, so the right method will
be chosen at link-time. This means that a target can only link with
either //blimp/client/core or //blimp/client/dummy. Safeguards are
added to ensure that //blimp/client/core is only visible outside of
//blimp/client if the GN argument enable_blimp has been set.

When using the BlimpClientContextFactory (either in Java or C++), the
right implementation (core or dummy) will always be chosen correctly
based on the link-time selection of the Create-method.

BUG=None
==========

[nyquist, 2016-07-12 22:05:01]

Description was changed from

==========
Add BlimpClientContext and factory with real and dummy implementation

The BlimpClientContext is the core class which wires up all of the blimp
functionality. When it's created, it will get all its dependencies from
the embedder. It is currently in charge of creating BlimpContents, but
will also own things like the BlimpClientSession. It is part of the
public //blimp/client/public API. It is a KeyedService and is typically
tied to a profile.

The BlimpClientContextFactory lives in //chrome to be able to access
Profile during construction, since //blimp/client currently does not
depend on //content, and there are upcoming dependencies that will
probably be implemented by classes in //chrome.

The BlimpClientContext has two implementations, one in
//blimp/client/core and one in //blimp/client/dummy. The dummy
implementation is to be used when we do not want the blimp
code to be used, and the core implementation is the real blimp
implementation.

The BlimpClientContext has a public static create-method that is not
implemented in //blimp/client/public, but instead it is implemented by
//blimp/client/core and //blimp/client/dummy, so the right method will
be chosen at link-time. This means that a target can only link with
either //blimp/client/core or //blimp/client/dummy. Safeguards are
added to ensure that //blimp/client/core is only visible outside of
//blimp/client if the GN argument enable_blimp has been set.

When using the BlimpClientContextFactory (either in Java or C++), the
right implementation (core or dummy) will always be chosen correctly
based on the link-time selection of the Create-method.

BUG=None
==========

to

==========
Add BlimpClientContext and factory with real and dummy implementation

The BlimpClientContext is the core class which wires up all of the blimp
functionality. When it's created, it will get all its dependencies from
the embedder. It is currently in charge of creating BlimpContents, but
will also own things like the BlimpClientSession. It is part of the
public //blimp/client/public API. It is a KeyedService and is typically
tied to a profile.

The BlimpClientContextFactory lives in //chrome to be able to access
Profile during construction, since //blimp/client currently does not
depend on //content, and there are upcoming dependencies that will
probably be implemented by classes in //chrome.

The BlimpClientContext has two implementations, one in
//blimp/client/core and one in //blimp/client/dummy. The dummy
implementation is to be used when we do not want the blimp
code to be used, and the core implementation is the real blimp
implementation.

The BlimpClientContext has a public static create-method that is not
implemented in //blimp/client/public, but instead it is implemented by
//blimp/client/core and //blimp/client/dummy, so the right method will
be chosen at link-time. This means that a target can only link with
either //blimp/client/core or //blimp/client/dummy. Safeguards are
added to ensure that //blimp/client/core is only visible outside of
//blimp/client if the GN argument enable_blimp has been set.

When using the BlimpClientContextFactory (either in Java or C++), the
right implementation (core or dummy) will always be chosen correctly
based on the link-time selection of the Create-method.

BUG=611094
==========

[nyquist, 2016-07-13 01:00:11]

Description was changed from

==========
Add BlimpClientContext and factory with real and dummy implementation

The BlimpClientContext is the core class which wires up all of the blimp
functionality. When it's created, it will get all its dependencies from
the embedder. It is currently in charge of creating BlimpContents, but
will also own things like the BlimpClientSession. It is part of the
public //blimp/client/public API. It is a KeyedService and is typically
tied to a profile.

The BlimpClientContextFactory lives in //chrome to be able to access
Profile during construction, since //blimp/client currently does not
depend on //content, and there are upcoming dependencies that will
probably be implemented by classes in //chrome.

The BlimpClientContext has two implementations, one in
//blimp/client/core and one in //blimp/client/dummy. The dummy
implementation is to be used when we do not want the blimp
code to be used, and the core implementation is the real blimp
implementation.

The BlimpClientContext has a public static create-method that is not
implemented in //blimp/client/public, but instead it is implemented by
//blimp/client/core and //blimp/client/dummy, so the right method will
be chosen at link-time. This means that a target can only link with
either //blimp/client/core or //blimp/client/dummy. Safeguards are
added to ensure that //blimp/client/core is only visible outside of
//blimp/client if the GN argument enable_blimp has been set.

When using the BlimpClientContextFactory (either in Java or C++), the
right implementation (core or dummy) will always be chosen correctly
based on the link-time selection of the Create-method.

BUG=611094
==========

to

==========
Add BlimpClientContext and factory with real and dummy implementation

The BlimpClientContext is the core class which wires up all of the blimp
functionality. When it's created, it will get all its dependencies from
the embedder. It is currently in charge of creating BlimpContents, but
will also own things like the BlimpClientSession. It is part of the
public //blimp/client/public API. It is a KeyedService and is typically
tied to a profile.

The BlimpClientContextFactory lives in //chrome to be able to access
Profile during construction, since //blimp/client currently does not
depend on //content, and there are upcoming dependencies that will
probably be implemented by classes in //chrome.

The BlimpClientContext has two implementations, one in
//blimp/client/core and one in //blimp/client/dummy. The dummy
implementation is to be used when we do not want the blimp
code to be used, and the core implementation is the real blimp
implementation.

The BlimpClientContext has a public static create-method that is not
implemented in //blimp/client/public, but instead it is implemented by
//blimp/client/core and //blimp/client/dummy, so the right method will
be chosen at link-time. This means that a target can only link with
either //blimp/client/core or //blimp/client/dummy. Safeguards are
added to ensure that //blimp/client/core is only visible outside of
//blimp/client if the GN argument enable_blimp has been set.

When using the BlimpClientContextFactory (either in Java or C++), the
right implementation (core or dummy) will always be chosen correctly
based on the link-time selection of the Create-method.

BUG=611094
==========

[nyquist, 2016-07-13 15:08:28]

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

[commit-bot: I haz the power, 2016-07-13 15:08:49]

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-13 15:14:07]

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

[commit-bot: I haz the power, 2016-07-13 15:14:08]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[nyquist, 2016-07-13 16:53:44]

nyquist@chromium.org changed reviewers:
+ dtrainor@chromium.org

[nyquist, 2016-07-13 16:53:45]

dtrainor: PTAL

I'll start working on adding the delegate instead of taking the context in the
create-method for the BlimpClientContext.

[nyquist, 2016-07-13 17:37:38]

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

[commit-bot: I haz the power, 2016-07-13 17:38:34]

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-13 18:29:00]

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

[commit-bot: I haz the power, 2016-07-13 18:29:01]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)

[nyquist, 2016-07-13 19:06:50]

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

[commit-bot: I haz the power, 2016-07-13 19:08:07]

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-13 19:26:18]

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

[commit-bot: I haz the power, 2016-07-13 19:26:19]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[nyquist, 2016-07-14 16:39:27]

Description was changed from

==========
Add BlimpClientContext and factory with real and dummy implementation

The BlimpClientContext is the core class which wires up all of the blimp
functionality. When it's created, it will get all its dependencies from
the embedder. It is currently in charge of creating BlimpContents, but
will also own things like the BlimpClientSession. It is part of the
public //blimp/client/public API. It is a KeyedService and is typically
tied to a profile.

The BlimpClientContextFactory lives in //chrome to be able to access
Profile during construction, since //blimp/client currently does not
depend on //content, and there are upcoming dependencies that will
probably be implemented by classes in //chrome.

The BlimpClientContext has two implementations, one in
//blimp/client/core and one in //blimp/client/dummy. The dummy
implementation is to be used when we do not want the blimp
code to be used, and the core implementation is the real blimp
implementation.

The BlimpClientContext has a public static create-method that is not
implemented in //blimp/client/public, but instead it is implemented by
//blimp/client/core and //blimp/client/dummy, so the right method will
be chosen at link-time. This means that a target can only link with
either //blimp/client/core or //blimp/client/dummy. Safeguards are
added to ensure that //blimp/client/core is only visible outside of
//blimp/client if the GN argument enable_blimp has been set.

When using the BlimpClientContextFactory (either in Java or C++), the
right implementation (core or dummy) will always be chosen correctly
based on the link-time selection of the Create-method.

BUG=611094
==========

to

==========
Add BlimpClientContext and factory with real and dummy implementation

The BlimpClientContext is the core class which wires up all of the blimp
functionality. When it's created, it will get all its dependencies from
the embedder. It is currently in charge of creating BlimpContents, but
will also own things like the BlimpClientSession. It is part of the
public //blimp/client/public API. It is a KeyedService and is typically
tied to a profile.

The BlimpClientContextFactory lives in //chrome to be able to access
Profile during construction, since //blimp/client currently does not
depend on //content, and there are upcoming dependencies that will
probably be implemented by classes in //chrome.

The BlimpClientContext has two implementations, one in
//blimp/client/core and one in //blimp/client/dummy. The dummy
implementation is to be used when we do not want the blimp
code to be used, and the core implementation is the real blimp
implementation.

The BlimpClientContext has a public static create-method that is not
implemented in //blimp/client/public, but instead it is implemented by
//blimp/client/core and //blimp/client/dummy, so the right method will
be chosen at link-time. This means that a target can only link with
either //blimp/client/core or //blimp/client/dummy. Safeguards are
added to ensure that //blimp/client/core is only visible outside of
//blimp/client if the GN argument enable_blimp has been set.

When using the BlimpClientContextFactory (either in Java or C++), the
right implementation (core or dummy) will always be chosen correctly
based on the link-time selection of the Create-method.

The BlimpClientContext has a delegate that it can call out to whenever
it needs specific functionality from the embedder. As an example, this
CL adds a helper for attaching a Profile to every created
BlimpContents to make have similar functionality as what is built into
WebContents so one can get a Profile and thereby services by only
having access to a BlimpContents.

BUG=611094
==========

[David Trainor- moved to gerrit, 2016-07-14 19:07:13]

pretty straightforward.  lgtm.

https://codereview.chromium.org/2132163002/diff/180001/blimp/client/core/blim...
File blimp/client/core/blimp_client_context_impl_unittest.cc (right):

https://codereview.chromium.org/2132163002/diff/180001/blimp/client/core/blim...
blimp/client/core/blimp_client_context_impl_unittest.cc:25:
delegate.GetBlimpContentsWithLastAttachedHelpers());
Should we use gmock for this?

https://codereview.chromium.org/2132163002/diff/180001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/blimp/ChromeBlimpClientContextDelegate.java
(right):

https://codereview.chromium.org/2132163002/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/blimp/ChromeBlimpClientContextDelegate.java:18:
public static ChromeBlimpClientContextDelegate
createAndSetDelegateForContext(Profile profile) {
Javadoc?  Explain a bit taht only one of these can be bound to the context at
the same time?  Should this also take in a BlimpClientContext instead of a
Profile?  Maybe to make it more explicit?

https://codereview.chromium.org/2132163002/diff/180001/chrome/browser/android...
File chrome/browser/android/blimp/blimp_contents_profile_attachment.h (right):

https://codereview.chromium.org/2132163002/diff/180001/chrome/browser/android...
chrome/browser/android/blimp/blimp_contents_profile_attachment.h:13: }
// namespaces?  I don't think they're necessary for small scopes like this, but
you do it in other places.

[David Trainor- moved to gerrit, 2016-07-14 19:08:58]

On 2016/07/14 19:07:13, David Trainor wrote:
> pretty straightforward.  lgtm.

On that note, I really like the class structure and the dummy setup.  This makes
integration really easy for Chrome for Android :).  Thanks for fleshing this
out!

> 
>
https://codereview.chromium.org/2132163002/diff/180001/blimp/client/core/blim...
> File blimp/client/core/blimp_client_context_impl_unittest.cc (right):
> 
>
https://codereview.chromium.org/2132163002/diff/180001/blimp/client/core/blim...
> blimp/client/core/blimp_client_context_impl_unittest.cc:25:
> delegate.GetBlimpContentsWithLastAttachedHelpers());
> Should we use gmock for this?
> 
>
https://codereview.chromium.org/2132163002/diff/180001/chrome/android/java/sr...
> File
>
chrome/android/java/src/org/chromium/chrome/browser/blimp/ChromeBlimpClientContextDelegate.java
> (right):
> 
>
https://codereview.chromium.org/2132163002/diff/180001/chrome/android/java/sr...
>
chrome/android/java/src/org/chromium/chrome/browser/blimp/ChromeBlimpClientContextDelegate.java:18:
> public static ChromeBlimpClientContextDelegate
> createAndSetDelegateForContext(Profile profile) {
> Javadoc?  Explain a bit taht only one of these can be bound to the context at
> the same time?  Should this also take in a BlimpClientContext instead of a
> Profile?  Maybe to make it more explicit?
> 
>
https://codereview.chromium.org/2132163002/diff/180001/chrome/browser/android...
> File chrome/browser/android/blimp/blimp_contents_profile_attachment.h (right):
> 
>
https://codereview.chromium.org/2132163002/diff/180001/chrome/browser/android...
> chrome/browser/android/blimp/blimp_contents_profile_attachment.h:13: }
> // namespaces?  I don't think they're necessary for small scopes like this,
but
> you do it in other places.

[nyquist, 2016-07-14 22:56:50]

all done

https://codereview.chromium.org/2132163002/diff/180001/blimp/client/core/blim...
File blimp/client/core/blimp_client_context_impl_unittest.cc (right):

https://codereview.chromium.org/2132163002/diff/180001/blimp/client/core/blim...
blimp/client/core/blimp_client_context_impl_unittest.cc:25:
delegate.GetBlimpContentsWithLastAttachedHelpers());
On 2016/07/14 19:07:13, David Trainor wrote:
> Should we use gmock for this?

I would love to! I'm not sure it's possible though, since the blimp_contents I
want to DCHECK against (or ... in the mock case, EXPECT_CALL against), is in
fact the return value. So I'm not sure how to correctly set up the expectations
before I call the CreateBlimpContents() method.

https://codereview.chromium.org/2132163002/diff/180001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/blimp/ChromeBlimpClientContextDelegate.java
(right):

https://codereview.chromium.org/2132163002/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/blimp/ChromeBlimpClientContextDelegate.java:18:
public static ChromeBlimpClientContextDelegate
createAndSetDelegateForContext(Profile profile) {
On 2016/07/14 19:07:13, David Trainor wrote:
> Javadoc?  Explain a bit taht only one of these can be bound to the context at
> the same time?  Should this also take in a BlimpClientContext instead of a
> Profile?  Maybe to make it more explicit?

Yeah, that'd be cool, but the constructor of the C++ version of the
ChromeBlimpClientContextDelegate needs the Profile, since it needs it to attach
the Profile to new BlimpContents. I /could/ pass both, but since the BCC can be
retrieved from the Profile, that feels weird too.

Anyway, I've of course added documentation for this method.

https://codereview.chromium.org/2132163002/diff/180001/chrome/browser/android...
File chrome/browser/android/blimp/blimp_contents_profile_attachment.h (right):

https://codereview.chromium.org/2132163002/diff/180001/chrome/browser/android...
chrome/browser/android/blimp/blimp_contents_profile_attachment.h:13: }
On 2016/07/14 19:07:13, David Trainor wrote:
> // namespaces?  I don't think they're necessary for small scopes like this,
but
> you do it in other places.

Done.

[nyquist, 2016-07-14 23:09:39]

nyquist@chromium.org changed reviewers:
+ dpranke@chromium.org, erg@chromium.org, thestig@chromium.org

[nyquist, 2016-07-14 23:09:40]

dpranke: PTAL //build/args/blimp_client.gn and also //chrome/browser/BUILD.gn
for guidance.

thestig: PTAL OWNERS for //chrome/browser/BUILD.gn

erg: PTAL //blimp/client/public/DEPS. Usage is in
blimp/client/public/blimp_client_context.h.

[Lei Zhang, 2016-07-14 23:21:58]

https://codereview.chromium.org/2132163002/diff/220001/chrome/browser/android...
File chrome/browser/android/blimp/blimp_client_context_factory.h (right):

https://codereview.chromium.org/2132163002/diff/220001/chrome/browser/android...
chrome/browser/android/blimp/blimp_client_context_factory.h:26: namespace chrome
{
BTW, a few years ago we decided chrome/ was top level, so we weren't going to
bother with namespace chrome. Not sure how much you've already added, but let's
not add more. https://bugs.chromium.org/p/chromium/issues/detail?id=289619

https://codereview.chromium.org/2132163002/diff/220001/chrome/browser/android...
File chrome/browser/android/blimp/blimp_contents_profile_attachment.cc (right):

https://codereview.chromium.org/2132163002/diff/220001/chrome/browser/android...
chrome/browser/android/blimp/blimp_contents_profile_attachment.cc:17: public:
Aren't struct members public by default?

[Dirk Pranke, 2016-07-14 23:25:51]

https://codereview.chromium.org/2132163002/diff/220001/blimp/client/core/BUIL...
File blimp/client/core/BUILD.gn (right):

https://codereview.chromium.org/2132163002/diff/220001/blimp/client/core/BUIL...
blimp/client/core/BUILD.gn:12: source_set("core") {
It seems like it might make more sense to move all of the //blimp/client/dummy
code into this directory, and just
provide the two different definitions of :core so that it's a little clearer
that they're related, e.g.:

if (enable_blimp) {
  source_set("core") { ... }
} else {
  source_set("core") { ... }
}

If you do that, I think you can also just inline the declare_args() block into
this file and not need blimp.gni. Then your consumers can unconditionally depend
on //blimp/client/core and not worry about whether blimp is enabled or not;
we've generally found that this gives you better modularity in the build files.

[nyquist, 2016-07-15 17:38:19]

dpranke, thestig: PTAL

https://codereview.chromium.org/2132163002/diff/220001/blimp/client/core/BUIL...
File blimp/client/core/BUILD.gn (right):

https://codereview.chromium.org/2132163002/diff/220001/blimp/client/core/BUIL...
blimp/client/core/BUILD.gn:12: source_set("core") {
On 2016/07/14 23:25:51, Dirk Pranke wrote:
> It seems like it might make more sense to move all of the //blimp/client/dummy
> code into this directory, and just
> provide the two different definitions of :core so that it's a little clearer
> that they're related, e.g.:
> 
> if (enable_blimp) {
>   source_set("core") { ... }
> } else {
>   source_set("core") { ... }
> }
> 
> If you do that, I think you can also just inline the declare_args() block into
> this file and not need blimp.gni. Then your consumers can unconditionally
depend
> on //blimp/client/core and not worry about whether blimp is enabled or not;
> we've generally found that this gives you better modularity in the build
files.

All done! Thanks for your help!

https://codereview.chromium.org/2132163002/diff/220001/chrome/browser/android...
File chrome/browser/android/blimp/blimp_client_context_factory.h (right):

https://codereview.chromium.org/2132163002/diff/220001/chrome/browser/android...
chrome/browser/android/blimp/blimp_client_context_factory.h:26: namespace chrome
{
On 2016/07/14 23:21:57, Lei Zhang wrote:
> BTW, a few years ago we decided chrome/ was top level, so we weren't going to
> bother with namespace chrome. Not sure how much you've already added, but
let's
> not add more. https://bugs.chromium.org/p/chromium/issues/detail?id=289619

Done. Thanks for the history! :-)

https://codereview.chromium.org/2132163002/diff/220001/chrome/browser/android...
File chrome/browser/android/blimp/blimp_contents_profile_attachment.cc (right):

https://codereview.chromium.org/2132163002/diff/220001/chrome/browser/android...
chrome/browser/android/blimp/blimp_contents_profile_attachment.cc:17: public:
On 2016/07/14 23:21:57, Lei Zhang wrote:
> Aren't struct members public by default?

Done.

[nyquist, 2016-07-15 18:13:25]

Description was changed from

==========
Add BlimpClientContext and factory with real and dummy implementation

The BlimpClientContext is the core class which wires up all of the blimp
functionality. When it's created, it will get all its dependencies from
the embedder. It is currently in charge of creating BlimpContents, but
will also own things like the BlimpClientSession. It is part of the
public //blimp/client/public API. It is a KeyedService and is typically
tied to a profile.

The BlimpClientContextFactory lives in //chrome to be able to access
Profile during construction, since //blimp/client currently does not
depend on //content, and there are upcoming dependencies that will
probably be implemented by classes in //chrome.

The BlimpClientContext has two implementations, one in
//blimp/client/core and one in //blimp/client/dummy. The dummy
implementation is to be used when we do not want the blimp
code to be used, and the core implementation is the real blimp
implementation.

The BlimpClientContext has a public static create-method that is not
implemented in //blimp/client/public, but instead it is implemented by
//blimp/client/core and //blimp/client/dummy, so the right method will
be chosen at link-time. This means that a target can only link with
either //blimp/client/core or //blimp/client/dummy. Safeguards are
added to ensure that //blimp/client/core is only visible outside of
//blimp/client if the GN argument enable_blimp has been set.

When using the BlimpClientContextFactory (either in Java or C++), the
right implementation (core or dummy) will always be chosen correctly
based on the link-time selection of the Create-method.

The BlimpClientContext has a delegate that it can call out to whenever
it needs specific functionality from the embedder. As an example, this
CL adds a helper for attaching a Profile to every created
BlimpContents to make have similar functionality as what is built into
WebContents so one can get a Profile and thereby services by only
having access to a BlimpContents.

BUG=611094
==========

to

==========
Add BlimpClientContext and factory with real and dummy implementation

The BlimpClientContext is the core class which wires up all of the blimp
functionality. When it's created, it will get all its dependencies from
the embedder. It is currently in charge of creating BlimpContents, but
will also own things like the BlimpClientSession. It is part of the
public //blimp/client/public API. It is a KeyedService and is typically
tied to a Profile.

The BlimpClientContextFactory lives in //chrome to be able to access
Profile during construction, since //blimp/client currently does not
depend on //content where BrowserContext lives, and there are upcoming
dependencies that will be implemented by classes in //chrome.

The BlimpClientContext has two implementations in
//blimp/client/core, an actual one and a dummy implementation. The
dummy implementation is to be used when we do not want the blimp
code to be used, and the core implementation is the real blimp
implementation. Which one of these are in use is controlled by the
GN argument enable_blimp which defaults to false.

The BlimpClientContext has a public static create-method that is not
implemented in //blimp/client/public, but instead it is implemented by
//blimp/client/core by both the actual and the dummy implementation,
so the right method will be chosen at link-time. This means that a
target can only link with either the actual or dummy implementation.
Everything in //blimp/client/core is only visible to //blimp/client/*
and embedders should only ever depend on //blimp/client/public, which
will bring in the correct implementation. This works the same for both
C++ and Java, so when using the BlimpClientContextFactory (either in
Java or C++), the right implementation (actual or dummy) will always
be chosen correctly based on the link-time selection of the
Create-method.

The BlimpClientContext has a delegate that it can call out to whenever
it needs specific functionality from the embedder. As an example, this
CL adds a helper for attaching a Profile to every created
BlimpContents to make have similar functionality as what is built into
WebContents so one can get a Profile and thereby services by only
having access to a BlimpContents.

BUG=611094
==========

[Dirk Pranke, 2016-07-15 18:31:21]

lgtm, much cleaner (though I just checked the BUILD.gn files).

https://codereview.chromium.org/2132163002/diff/240001/blimp/client/core/BUIL...
File blimp/client/core/BUILD.gn (right):

https://codereview.chromium.org/2132163002/diff/240001/blimp/client/core/BUIL...
blimp/client/core/BUILD.gn:16: # This target is here to facilitate //chrome to
depend on //blimp/client/public
Nit: I might reword this as "here to let //chrome depend on
//blimp/client/public".

https://codereview.chromium.org/2132163002/diff/240001/blimp/client/core/BUIL...
blimp/client/core/BUILD.gn:117: # //blimp/client/public:public_java and
automatically get the correct
same.

https://codereview.chromium.org/2132163002/diff/240001/blimp/client/public/BU...
File blimp/client/public/BUILD.gn (right):

https://codereview.chromium.org/2132163002/diff/240001/blimp/client/public/BU...
blimp/client/public/BUILD.gn:11: # instead use
//blimp/client/public:public_headers directly.
I might add something like "This target differs from :public_headers by pulling
in the appropriate implementation as well as the headers".

https://codereview.chromium.org/2132163002/diff/240001/blimp/client/public/BU...
blimp/client/public/BUILD.gn:62: android_library("public_headers_java") {
is "interfaces" more appropriate than "headers" here?

[Lei Zhang, 2016-07-15 19:01:28]

lgtm

[nyquist, 2016-07-15 19:03:54]

All done!

https://codereview.chromium.org/2132163002/diff/240001/blimp/client/core/BUIL...
File blimp/client/core/BUILD.gn (right):

https://codereview.chromium.org/2132163002/diff/240001/blimp/client/core/BUIL...
blimp/client/core/BUILD.gn:16: # This target is here to facilitate //chrome to
depend on //blimp/client/public
On 2016/07/15 18:31:21, Dirk Pranke wrote:
> Nit: I might reword this as "here to let //chrome depend on
> //blimp/client/public".

Done.

https://codereview.chromium.org/2132163002/diff/240001/blimp/client/core/BUIL...
blimp/client/core/BUILD.gn:117: # //blimp/client/public:public_java and
automatically get the correct
On 2016/07/15 18:31:21, Dirk Pranke wrote:
> same.

Done.

https://codereview.chromium.org/2132163002/diff/240001/blimp/client/public/BU...
File blimp/client/public/BUILD.gn (right):

https://codereview.chromium.org/2132163002/diff/240001/blimp/client/public/BU...
blimp/client/public/BUILD.gn:11: # instead use
//blimp/client/public:public_headers directly.
On 2016/07/15 18:31:21, Dirk Pranke wrote:
> I might add something like "This target differs from :public_headers by
pulling
> in the appropriate implementation as well as the headers".

Done.

https://codereview.chromium.org/2132163002/diff/240001/blimp/client/public/BU...
blimp/client/public/BUILD.gn:62: android_library("public_headers_java") {
On 2016/07/15 18:31:21, Dirk Pranke wrote:
> is "interfaces" more appropriate than "headers" here? 

Discussed internally in the team, and we decided to keep it as 'headers' to make
it simpler to understand the structure across both Java an C++. Hope that's OK.

[nyquist, 2016-07-15 19:05:55]

Description was changed from

==========
Add BlimpClientContext and factory with real and dummy implementation

The BlimpClientContext is the core class which wires up all of the blimp
functionality. When it's created, it will get all its dependencies from
the embedder. It is currently in charge of creating BlimpContents, but
will also own things like the BlimpClientSession. It is part of the
public //blimp/client/public API. It is a KeyedService and is typically
tied to a Profile.

The BlimpClientContextFactory lives in //chrome to be able to access
Profile during construction, since //blimp/client currently does not
depend on //content where BrowserContext lives, and there are upcoming
dependencies that will be implemented by classes in //chrome.

The BlimpClientContext has two implementations in
//blimp/client/core, an actual one and a dummy implementation. The
dummy implementation is to be used when we do not want the blimp
code to be used, and the core implementation is the real blimp
implementation. Which one of these are in use is controlled by the
GN argument enable_blimp which defaults to false.

The BlimpClientContext has a public static create-method that is not
implemented in //blimp/client/public, but instead it is implemented by
//blimp/client/core by both the actual and the dummy implementation,
so the right method will be chosen at link-time. This means that a
target can only link with either the actual or dummy implementation.
Everything in //blimp/client/core is only visible to //blimp/client/*
and embedders should only ever depend on //blimp/client/public, which
will bring in the correct implementation. This works the same for both
C++ and Java, so when using the BlimpClientContextFactory (either in
Java or C++), the right implementation (actual or dummy) will always
be chosen correctly based on the link-time selection of the
Create-method.

The BlimpClientContext has a delegate that it can call out to whenever
it needs specific functionality from the embedder. As an example, this
CL adds a helper for attaching a Profile to every created
BlimpContents to make have similar functionality as what is built into
WebContents so one can get a Profile and thereby services by only
having access to a BlimpContents.

BUG=611094
==========

to

==========
Add BlimpClientContext and factory with real and dummy implementation

The BlimpClientContext is the core class which wires up all of the blimp
functionality. When it's created, it will get all its dependencies from
the embedder. It is currently in charge of creating BlimpContents, but
will also own things like the BlimpClientSession. It is part of the
public //blimp/client/public API. It is a KeyedService and is typically
tied to a Profile.

The BlimpClientContextFactory lives in //chrome to be able to access
Profile during construction, since //blimp/client currently does not
depend on //content where BrowserContext lives, and there are upcoming
dependencies that will be implemented by classes in //chrome.

The BlimpClientContext has two implementations in
//blimp/client/core, an actual one and a dummy implementation. The
dummy implementation is to be used when we do not want the blimp
code to be used, and the core implementation is the real blimp
implementation. Which one of these are in use is controlled by the
GN argument enable_blimp which defaults to false.

The BlimpClientContext has a public static create-method that is not
implemented in //blimp/client/public, but instead it is implemented by
//blimp/client/core by both the actual and the dummy implementation,
so the right method will be chosen at link-time. This means that a
target can only link with either the actual or dummy implementation.
Everything in //blimp/client/core is only visible to //blimp/client/*
and embedders should only ever depend on //blimp/client/public, which
will bring in the correct implementation. This works the same for both
C++ and Java, so when using the BlimpClientContextFactory (either in
Java or C++), the right implementation (actual or dummy) will always
be chosen correctly based on the link-time selection of the
Create-method.

The BlimpClientContext has a delegate that it can call out to whenever
it needs specific functionality from the embedder. As an example, this
CL adds a helper for attaching a Profile to every created
BlimpContents to make have similar functionality as what is built into
WebContents so one can get a Profile and thereby services by only
having access to a BlimpContents.

BUG=611094
TBR=erg@chromium.org
==========

[nyquist, 2016-07-15 19:06:50]

erg: TBRing for the DEPS on //components/keyed_service/core since we discussed
this exact CL offline before I started it.

Happy to address any comments you might have in a follow-up CL.

[Dirk Pranke, 2016-07-15 19:07:59]

https://codereview.chromium.org/2132163002/diff/240001/blimp/client/public/BU...
File blimp/client/public/BUILD.gn (right):

https://codereview.chromium.org/2132163002/diff/240001/blimp/client/public/BU...
blimp/client/public/BUILD.gn:62: android_library("public_headers_java") {
On 2016/07/15 19:03:54, nyquist wrote:
> On 2016/07/15 18:31:21, Dirk Pranke wrote:
> > is "interfaces" more appropriate than "headers" here? 
> 
> Discussed internally in the team, and we decided to keep it as 'headers' to
make
> it simpler to understand the structure across both Java an C++. Hope that's
OK.

Yup, that's fine.

[nyquist, 2016-07-15 19:22:03]

The CQ bit was checked by nyquist@chromium.org

[nyquist, 2016-07-15 19:22:04]

The patchset sent to the CQ was uploaded after l-g-t-m from
dtrainor@chromium.org, thestig@chromium.org, dpranke@chromium.org
Link to the patchset: https://codereview.chromium.org/2132163002/#ps280001
(title: "git merge origin/master before CQ for good measure")

[commit-bot: I haz the power, 2016-07-15 19:22:38]

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-15 21:20:39]

Description was changed from

==========
Add BlimpClientContext and factory with real and dummy implementation

The BlimpClientContext is the core class which wires up all of the blimp
functionality. When it's created, it will get all its dependencies from
the embedder. It is currently in charge of creating BlimpContents, but
will also own things like the BlimpClientSession. It is part of the
public //blimp/client/public API. It is a KeyedService and is typically
tied to a Profile.

The BlimpClientContextFactory lives in //chrome to be able to access
Profile during construction, since //blimp/client currently does not
depend on //content where BrowserContext lives, and there are upcoming
dependencies that will be implemented by classes in //chrome.

The BlimpClientContext has two implementations in
//blimp/client/core, an actual one and a dummy implementation. The
dummy implementation is to be used when we do not want the blimp
code to be used, and the core implementation is the real blimp
implementation. Which one of these are in use is controlled by the
GN argument enable_blimp which defaults to false.

The BlimpClientContext has a public static create-method that is not
implemented in //blimp/client/public, but instead it is implemented by
//blimp/client/core by both the actual and the dummy implementation,
so the right method will be chosen at link-time. This means that a
target can only link with either the actual or dummy implementation.
Everything in //blimp/client/core is only visible to //blimp/client/*
and embedders should only ever depend on //blimp/client/public, which
will bring in the correct implementation. This works the same for both
C++ and Java, so when using the BlimpClientContextFactory (either in
Java or C++), the right implementation (actual or dummy) will always
be chosen correctly based on the link-time selection of the
Create-method.

The BlimpClientContext has a delegate that it can call out to whenever
it needs specific functionality from the embedder. As an example, this
CL adds a helper for attaching a Profile to every created
BlimpContents to make have similar functionality as what is built into
WebContents so one can get a Profile and thereby services by only
having access to a BlimpContents.

BUG=611094
TBR=erg@chromium.org
==========

to

==========
Add BlimpClientContext and factory with real and dummy implementation

The BlimpClientContext is the core class which wires up all of the blimp
functionality. When it's created, it will get all its dependencies from
the embedder. It is currently in charge of creating BlimpContents, but
will also own things like the BlimpClientSession. It is part of the
public //blimp/client/public API. It is a KeyedService and is typically
tied to a Profile.

The BlimpClientContextFactory lives in //chrome to be able to access
Profile during construction, since //blimp/client currently does not
depend on //content where BrowserContext lives, and there are upcoming
dependencies that will be implemented by classes in //chrome.

The BlimpClientContext has two implementations in
//blimp/client/core, an actual one and a dummy implementation. The
dummy implementation is to be used when we do not want the blimp
code to be used, and the core implementation is the real blimp
implementation. Which one of these are in use is controlled by the
GN argument enable_blimp which defaults to false.

The BlimpClientContext has a public static create-method that is not
implemented in //blimp/client/public, but instead it is implemented by
//blimp/client/core by both the actual and the dummy implementation,
so the right method will be chosen at link-time. This means that a
target can only link with either the actual or dummy implementation.
Everything in //blimp/client/core is only visible to //blimp/client/*
and embedders should only ever depend on //blimp/client/public, which
will bring in the correct implementation. This works the same for both
C++ and Java, so when using the BlimpClientContextFactory (either in
Java or C++), the right implementation (actual or dummy) will always
be chosen correctly based on the link-time selection of the
Create-method.

The BlimpClientContext has a delegate that it can call out to whenever
it needs specific functionality from the embedder. As an example, this
CL adds a helper for attaching a Profile to every created
BlimpContents to make have similar functionality as what is built into
WebContents so one can get a Profile and thereby services by only
having access to a BlimpContents.

BUG=611094
TBR=erg@chromium.org
==========

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

Committed patchset #15 (id:280001)

[commit-bot: I haz the power, 2016-07-15 21:21:05]

CQ bit was unchecked.

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

Description was changed from

==========
Add BlimpClientContext and factory with real and dummy implementation

The BlimpClientContext is the core class which wires up all of the blimp
functionality. When it's created, it will get all its dependencies from
the embedder. It is currently in charge of creating BlimpContents, but
will also own things like the BlimpClientSession. It is part of the
public //blimp/client/public API. It is a KeyedService and is typically
tied to a Profile.

The BlimpClientContextFactory lives in //chrome to be able to access
Profile during construction, since //blimp/client currently does not
depend on //content where BrowserContext lives, and there are upcoming
dependencies that will be implemented by classes in //chrome.

The BlimpClientContext has two implementations in
//blimp/client/core, an actual one and a dummy implementation. The
dummy implementation is to be used when we do not want the blimp
code to be used, and the core implementation is the real blimp
implementation. Which one of these are in use is controlled by the
GN argument enable_blimp which defaults to false.

The BlimpClientContext has a public static create-method that is not
implemented in //blimp/client/public, but instead it is implemented by
//blimp/client/core by both the actual and the dummy implementation,
so the right method will be chosen at link-time. This means that a
target can only link with either the actual or dummy implementation.
Everything in //blimp/client/core is only visible to //blimp/client/*
and embedders should only ever depend on //blimp/client/public, which
will bring in the correct implementation. This works the same for both
C++ and Java, so when using the BlimpClientContextFactory (either in
Java or C++), the right implementation (actual or dummy) will always
be chosen correctly based on the link-time selection of the
Create-method.

The BlimpClientContext has a delegate that it can call out to whenever
it needs specific functionality from the embedder. As an example, this
CL adds a helper for attaching a Profile to every created
BlimpContents to make have similar functionality as what is built into
WebContents so one can get a Profile and thereby services by only
having access to a BlimpContents.

BUG=611094
TBR=erg@chromium.org
==========

to

==========
Add BlimpClientContext and factory with real and dummy implementation

The BlimpClientContext is the core class which wires up all of the blimp
functionality. When it's created, it will get all its dependencies from
the embedder. It is currently in charge of creating BlimpContents, but
will also own things like the BlimpClientSession. It is part of the
public //blimp/client/public API. It is a KeyedService and is typically
tied to a Profile.

The BlimpClientContextFactory lives in //chrome to be able to access
Profile during construction, since //blimp/client currently does not
depend on //content where BrowserContext lives, and there are upcoming
dependencies that will be implemented by classes in //chrome.

The BlimpClientContext has two implementations in
//blimp/client/core, an actual one and a dummy implementation. The
dummy implementation is to be used when we do not want the blimp
code to be used, and the core implementation is the real blimp
implementation. Which one of these are in use is controlled by the
GN argument enable_blimp which defaults to false.

The BlimpClientContext has a public static create-method that is not
implemented in //blimp/client/public, but instead it is implemented by
//blimp/client/core by both the actual and the dummy implementation,
so the right method will be chosen at link-time. This means that a
target can only link with either the actual or dummy implementation.
Everything in //blimp/client/core is only visible to //blimp/client/*
and embedders should only ever depend on //blimp/client/public, which
will bring in the correct implementation. This works the same for both
C++ and Java, so when using the BlimpClientContextFactory (either in
Java or C++), the right implementation (actual or dummy) will always
be chosen correctly based on the link-time selection of the
Create-method.

The BlimpClientContext has a delegate that it can call out to whenever
it needs specific functionality from the embedder. As an example, this
CL adds a helper for attaching a Profile to every created
BlimpContents to make have similar functionality as what is built into
WebContents so one can get a Profile and thereby services by only
having access to a BlimpContents.

BUG=611094
TBR=erg@chromium.org

Committed: https://crrev.com/9aa0cdee23229ca95a3bea79abcc584a395d016e
Cr-Commit-Position: refs/heads/master@{#405860}
==========

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

Patchset 15 (id:??) landed as
https://crrev.com/9aa0cdee23229ca95a3bea79abcc584a395d016e
Cr-Commit-Position: refs/heads/master@{#405860}