[Blimp] Adds Blimp EngineSession and ClientSession skeleton

[Blimp] Adds Blimp EngineSession and ClientSession skeleton.

BlimpClientSessionManager (to be added) authenticates and creates client sessions. Once a client session is authenticated, it is given to the engine session. There is at most one active client session attached to the engine session.

The engine session manages a list of web contents.

Committed: https://crrev.com/28516c7ad6686f548c1c7f971b62c5f359f37b53
Cr-Commit-Position: refs/heads/master@{#354388}

[haibinlu, 2015-10-14 01:41:45]

haibinlu@chromium.org changed reviewers:
+ marshallk@google.com

[Kevin M, 2015-10-14 16:44:48]

kmarshall@chromium.org changed reviewers:
+ kmarshall@chromium.org
- marshallk@google.com

[Kevin M, 2015-10-14 17:52:57]

https://codereview.chromium.org/1403083002/diff/20001/blimp/engine/browser/bl...
File blimp/engine/browser/blimp_browser_main_parts.cc (right):

https://codereview.chromium.org/1403083002/diff/20001/blimp/engine/browser/bl...
blimp/engine/browser/blimp_browser_main_parts.cc:9: #include
"blimp/engine/browser/blimp_client_session_manager_delegate.h"
Not used

https://codereview.chromium.org/1403083002/diff/20001/blimp/engine/browser/bl...
blimp/engine/browser/blimp_browser_main_parts.cc:16: #include
"ui/base/ime/input_method_initializer.h"
Not used

https://codereview.chromium.org/1403083002/diff/20001/blimp/engine/browser/bl...
blimp/engine/browser/blimp_browser_main_parts.cc:40: engine_session_.reset();
Is this necessary? These will be destroyed automatically when |this| is
destroyed.

https://codereview.chromium.org/1403083002/diff/20001/blimp/engine/browser/bl...
blimp/engine/browser/blimp_browser_main_parts.cc:44: DCHECK(engine_session_);
This can be kept as a simple getter - the dcheck isn't adding anything here. If
engine_session_ is NULL, the dereference will segfault immediately anyways.

https://codereview.chromium.org/1403083002/diff/20001/blimp/engine/browser/bl...
File blimp/engine/browser/blimp_browser_main_parts.h (right):

https://codereview.chromium.org/1403083002/diff/20001/blimp/engine/browser/bl...
blimp/engine/browser/blimp_browser_main_parts.h:11: #include
"content/public/common/main_function_params.h"
Prototype MainFunctionParams?

https://codereview.chromium.org/1403083002/diff/20001/blimp/engine/browser/bl...
blimp/engine/browser/blimp_browser_main_parts.h:33: BlimpBrowserContext*
browser_context();
Need to use CamelCase naming for non-trivial getters

https://codereview.chromium.org/1403083002/diff/20001/blimp/engine/browser/bl...
File blimp/engine/browser/blimp_client_session.h (right):

https://codereview.chromium.org/1403083002/diff/20001/blimp/engine/browser/bl...
blimp/engine/browser/blimp_client_session.h:5: #ifndef
BLIMP_ENGINE_BROWSER_BLIMP_CLIENT_SESSION_H_
This, and the ClientSessionManager, should probably go under "net".

https://codereview.chromium.org/1403083002/diff/20001/blimp/engine/browser/bl...
blimp/engine/browser/blimp_client_session.h:22: // A blimp client has one
screen.
Remove these fields and includes as per our discussion.

https://codereview.chromium.org/1403083002/diff/20001/blimp/engine/browser/bl...
File blimp/engine/browser/blimp_client_session_manager.cc (right):

https://codereview.chromium.org/1403083002/diff/20001/blimp/engine/browser/bl...
blimp/engine/browser/blimp_client_session_manager.cc:16: namespace engine {
Since we decided on using the Blimp prefix for class names, we probably won't
need subnamespaces anymore.

https://codereview.chromium.org/1403083002/diff/20001/blimp/engine/browser/bl...
blimp/engine/browser/blimp_client_session_manager.cc:22: GURL GetStartupURL() {
Command line parsing should be in or around the main function, not this deep.

https://codereview.chromium.org/1403083002/diff/20001/blimp/engine/browser/bl...
blimp/engine/browser/blimp_client_session_manager.cc:45: DCHECK(delegate_);
Test support code doesn't belong here. Factor out session construction into the
caller or a test support helper fn.

https://codereview.chromium.org/1403083002/diff/20001/blimp/engine/browser/bl...
File blimp/engine/browser/blimp_client_session_manager.h (right):

https://codereview.chromium.org/1403083002/diff/20001/blimp/engine/browser/bl...
blimp/engine/browser/blimp_client_session_manager.h:30: void
AttachTestClientSession();
Recommend adding a separate testing constructor that accepts a
scoped_ptr<BlimpClientSession> test_session. It doesn't make much sense to allow
the client session to be attached at arbitrary points in the BCSM's lifetime.

https://codereview.chromium.org/1403083002/diff/20001/blimp/engine/browser/bl...
File blimp/engine/browser/blimp_client_session_manager_delegate.h (right):

https://codereview.chromium.org/1403083002/diff/20001/blimp/engine/browser/bl...
blimp/engine/browser/blimp_client_session_manager_delegate.h:20: virtual void
AttachClientSession(BlimpClientSession* client_session) = 0;
I recommend rephrasing these to "ClientSessionCreated" and
"ClientSessionDestroyed".

It's up to the delegate subclass to decide what to do with that session
(attach/detach).

https://codereview.chromium.org/1403083002/diff/20001/blimp/engine/browser/bl...
File blimp/engine/browser/blimp_engine_session.h (right):

https://codereview.chromium.org/1403083002/diff/20001/blimp/engine/browser/bl...
blimp/engine/browser/blimp_engine_session.h:8: #include <string>
not used

https://codereview.chromium.org/1403083002/diff/20001/blimp/engine/browser/bl...
blimp/engine/browser/blimp_engine_session.h:15: #include
"content/public/browser/web_contents_observer.h"
not used

https://codereview.chromium.org/1403083002/diff/20001/blimp/engine/browser/bl...
blimp/engine/browser/blimp_engine_session.h:45: BlimpBrowserContext*
browser_context() { return browser_context_.get(); }
Move this above the overrides

https://codereview.chromium.org/1403083002/diff/20001/blimp/engine/browser/bl...
blimp/engine/browser/blimp_engine_session.h:73: BlimpClientSession*
client_session_ = nullptr;
:-O

Wow, this is allowed now. Yay.

[haibinlu, 2015-10-15 01:59:28]

PTAL

https://codereview.chromium.org/1403083002/diff/20001/blimp/engine/browser/bl...
File blimp/engine/browser/blimp_browser_main_parts.cc (right):

https://codereview.chromium.org/1403083002/diff/20001/blimp/engine/browser/bl...
blimp/engine/browser/blimp_browser_main_parts.cc:9: #include
"blimp/engine/browser/blimp_client_session_manager_delegate.h"
On 2015/10/14 17:52:56, Kevin M wrote:
> Not used

Done.

https://codereview.chromium.org/1403083002/diff/20001/blimp/engine/browser/bl...
blimp/engine/browser/blimp_browser_main_parts.cc:16: #include
"ui/base/ime/input_method_initializer.h"
On 2015/10/14 17:52:56, Kevin M wrote:
> Not used

Done.

https://codereview.chromium.org/1403083002/diff/20001/blimp/engine/browser/bl...
blimp/engine/browser/blimp_browser_main_parts.cc:40: engine_session_.reset();
On 2015/10/14 17:52:56, Kevin M wrote:
> Is this necessary? These will be destroyed automatically when |this| is
> destroyed.

per offline discussion, it's for cleaning up objects that may depend on threads
that will be stopped after this method.

https://codereview.chromium.org/1403083002/diff/20001/blimp/engine/browser/bl...
blimp/engine/browser/blimp_browser_main_parts.cc:44: DCHECK(engine_session_);
On 2015/10/14 17:52:56, Kevin M wrote:
> This can be kept as a simple getter - the dcheck isn't adding anything here.
If
> engine_session_ is NULL, the dereference will segfault immediately anyways.

Done.

https://codereview.chromium.org/1403083002/diff/20001/blimp/engine/browser/bl...
File blimp/engine/browser/blimp_browser_main_parts.h (right):

https://codereview.chromium.org/1403083002/diff/20001/blimp/engine/browser/bl...
blimp/engine/browser/blimp_browser_main_parts.h:11: #include
"content/public/common/main_function_params.h"
On 2015/10/14 17:52:56, Kevin M wrote:
> Prototype MainFunctionParams?

Done.

https://codereview.chromium.org/1403083002/diff/20001/blimp/engine/browser/bl...
blimp/engine/browser/blimp_browser_main_parts.h:33: BlimpBrowserContext*
browser_context();
On 2015/10/14 17:52:56, Kevin M wrote:
> Need to use CamelCase naming for non-trivial getters

Done.

https://codereview.chromium.org/1403083002/diff/20001/blimp/engine/browser/bl...
File blimp/engine/browser/blimp_client_session.h (right):

https://codereview.chromium.org/1403083002/diff/20001/blimp/engine/browser/bl...
blimp/engine/browser/blimp_client_session.h:5: #ifndef
BLIMP_ENGINE_BROWSER_BLIMP_CLIENT_SESSION_H_
On 2015/10/14 17:52:56, Kevin M wrote:
> This, and the ClientSessionManager, should probably go under "net".

done. removed client session manager per offline discussion.

https://codereview.chromium.org/1403083002/diff/20001/blimp/engine/browser/bl...
blimp/engine/browser/blimp_client_session.h:22: // A blimp client has one
screen.
On 2015/10/14 17:52:56, Kevin M wrote:
> Remove these fields and includes as per our discussion.

done

https://codereview.chromium.org/1403083002/diff/20001/blimp/engine/browser/bl...
File blimp/engine/browser/blimp_client_session_manager.cc (right):

https://codereview.chromium.org/1403083002/diff/20001/blimp/engine/browser/bl...
blimp/engine/browser/blimp_client_session_manager.cc:16: namespace engine {
On 2015/10/14 17:52:56, Kevin M wrote:
> Since we decided on using the Blimp prefix for class names, we probably won't
> need subnamespaces anymore.

'engine' namespace is used through the engine folder. we can discuss this and
remove it if everybody agrees.

https://codereview.chromium.org/1403083002/diff/20001/blimp/engine/browser/bl...
blimp/engine/browser/blimp_client_session_manager.cc:22: GURL GetStartupURL() {
On 2015/10/14 17:52:57, Kevin M wrote:
> Command line parsing should be in or around the main function, not this deep.

moved to main_parts. this is just for dev purpose before we can pass in URL from
the client.

https://codereview.chromium.org/1403083002/diff/20001/blimp/engine/browser/bl...
blimp/engine/browser/blimp_client_session_manager.cc:45: DCHECK(delegate_);
On 2015/10/14 17:52:56, Kevin M wrote:
> Test support code doesn't belong here. Factor out session construction into
the
> caller or a test support helper fn.

done per offline discussion

https://codereview.chromium.org/1403083002/diff/20001/blimp/engine/browser/bl...
File blimp/engine/browser/blimp_client_session_manager.h (right):

https://codereview.chromium.org/1403083002/diff/20001/blimp/engine/browser/bl...
blimp/engine/browser/blimp_client_session_manager.h:30: void
AttachTestClientSession();
On 2015/10/14 17:52:57, Kevin M wrote:
> Recommend adding a separate testing constructor that accepts a
> scoped_ptr<BlimpClientSession> test_session. It doesn't make much sense to
allow
> the client session to be attached at arbitrary points in the BCSM's lifetime.

Done.

https://codereview.chromium.org/1403083002/diff/20001/blimp/engine/browser/bl...
File blimp/engine/browser/blimp_client_session_manager_delegate.h (right):

https://codereview.chromium.org/1403083002/diff/20001/blimp/engine/browser/bl...
blimp/engine/browser/blimp_client_session_manager_delegate.h:20: virtual void
AttachClientSession(BlimpClientSession* client_session) = 0;
On 2015/10/14 17:52:57, Kevin M wrote:
> I recommend rephrasing these to "ClientSessionCreated" and
> "ClientSessionDestroyed".
> 
> It's up to the delegate subclass to decide what to do with that session
> (attach/detach).

per offline discussion, rename this class to 'handler' and keep the method name.

https://codereview.chromium.org/1403083002/diff/20001/blimp/engine/browser/bl...
File blimp/engine/browser/blimp_engine_session.h (right):

https://codereview.chromium.org/1403083002/diff/20001/blimp/engine/browser/bl...
blimp/engine/browser/blimp_engine_session.h:8: #include <string>
On 2015/10/14 17:52:57, Kevin M wrote:
> not used

Done.

https://codereview.chromium.org/1403083002/diff/20001/blimp/engine/browser/bl...
blimp/engine/browser/blimp_engine_session.h:15: #include
"content/public/browser/web_contents_observer.h"
On 2015/10/14 17:52:57, Kevin M wrote:
> not used

Done.

https://codereview.chromium.org/1403083002/diff/20001/blimp/engine/browser/bl...
blimp/engine/browser/blimp_engine_session.h:45: BlimpBrowserContext*
browser_context() { return browser_context_.get(); }
On 2015/10/14 17:52:57, Kevin M wrote:
> Move this above the overrides

Done.

https://codereview.chromium.org/1403083002/diff/20001/blimp/engine/browser/bl...
blimp/engine/browser/blimp_engine_session.h:73: BlimpClientSession*
client_session_ = nullptr;
On 2015/10/14 17:52:57, Kevin M wrote:
> :-O
> 
> Wow, this is allowed now. Yay.

Acknowledged.

[Kevin M, 2015-10-15 17:14:04]

https://codereview.chromium.org/1403083002/diff/60001/blimp/engine/browser/bl...
File blimp/engine/browser/blimp_engine_session.cc (right):

https://codereview.chromium.org/1403083002/diff/60001/blimp/engine/browser/bl...
blimp/engine/browser/blimp_engine_session.cc:35: if (!client_session)
Add NOTIMPLEMENTED() here

https://codereview.chromium.org/1403083002/diff/60001/blimp/engine/browser/bl...
blimp/engine/browser/blimp_engine_session.cc:37:
client_session_.reset(client_session.release());
client_session_ = client_session.Pass() is safer/more idiomatic

https://codereview.chromium.org/1403083002/diff/60001/blimp/engine/browser/bl...
blimp/engine/browser/blimp_engine_session.cc:68: if
(message.client_control().type() == ClientControlMessage::NEW_TAB)
Put this in a switch statement - this list will grow pretty long over time.

https://codereview.chromium.org/1403083002/diff/60001/blimp/engine/browser/bl...
blimp/engine/browser/blimp_engine_session.cc:88: if (params.disposition !=
CURRENT_TAB)
Add TODO and NOTIMPLEMENTED

https://codereview.chromium.org/1403083002/diff/60001/blimp/engine/browser/bl...
blimp/engine/browser/blimp_engine_session.cc:100: if
(params.transferred_global_request_id != content::GlobalRequestID()) {
What does this block do? Comment plz

https://codereview.chromium.org/1403083002/diff/60001/blimp/engine/browser/bl...
blimp/engine/browser/blimp_engine_session.cc:134: content::WebContents*
new_contents) {
Take a scoped_ptr to make ownership transfer explicit

https://codereview.chromium.org/1403083002/diff/60001/blimp/engine/browser/bl...
File blimp/engine/browser/blimp_engine_session.h (right):

https://codereview.chromium.org/1403083002/diff/60001/blimp/engine/browser/bl...
blimp/engine/browser/blimp_engine_session.h:45: void CreateWebContents(const
GURL& url);
It feels strange that NEW_TAB is so conflated with navigating to a URL. Can't
they just be two separate steps?

[Kevin M, 2015-10-15 20:07:01]

Can you add some unit tests for these wherever possible?

[haibinlu, 2015-10-15 21:17:53]

PTAL

I need to figure out proper unit tests for this headless content shell. I will
use next CL to add these tests.

https://codereview.chromium.org/1403083002/diff/60001/blimp/engine/browser/bl...
File blimp/engine/browser/blimp_engine_session.cc (right):

https://codereview.chromium.org/1403083002/diff/60001/blimp/engine/browser/bl...
blimp/engine/browser/blimp_engine_session.cc:35: if (!client_session)
On 2015/10/15 17:14:03, Kevin M wrote:
> Add NOTIMPLEMENTED() here

Done.

https://codereview.chromium.org/1403083002/diff/60001/blimp/engine/browser/bl...
blimp/engine/browser/blimp_engine_session.cc:37:
client_session_.reset(client_session.release());
On 2015/10/15 17:14:03, Kevin M wrote:
> client_session_ = client_session.Pass() is safer/more idiomatic

Done.

https://codereview.chromium.org/1403083002/diff/60001/blimp/engine/browser/bl...
blimp/engine/browser/blimp_engine_session.cc:68: if
(message.client_control().type() == ClientControlMessage::NEW_TAB)
On 2015/10/15 17:14:03, Kevin M wrote:
> Put this in a switch statement - this list will grow pretty long over time.

Done.

https://codereview.chromium.org/1403083002/diff/60001/blimp/engine/browser/bl...
blimp/engine/browser/blimp_engine_session.cc:88: if (params.disposition !=
CURRENT_TAB)
On 2015/10/15 17:14:03, Kevin M wrote:
> Add TODO and NOTIMPLEMENTED

Done.

https://codereview.chromium.org/1403083002/diff/60001/blimp/engine/browser/bl...
blimp/engine/browser/blimp_engine_session.cc:100: if
(params.transferred_global_request_id != content::GlobalRequestID()) {
On 2015/10/15 17:14:03, Kevin M wrote:
> What does this block do? Comment plz

Done.

https://codereview.chromium.org/1403083002/diff/60001/blimp/engine/browser/bl...
blimp/engine/browser/blimp_engine_session.cc:134: content::WebContents*
new_contents) {
On 2015/10/15 17:14:03, Kevin M wrote:
> Take a scoped_ptr to make ownership transfer explicit

Done.

[Kevin M, 2015-10-15 22:01:56]

lgtm

https://codereview.chromium.org/1403083002/diff/100001/blimp/common/proto/con...
File blimp/common/proto/control.proto (right):

https://codereview.chromium.org/1403083002/diff/100001/blimp/common/proto/con...
blimp/common/proto/control.proto:5: // Message definitions for
client-originating browser control messages.
Not client-originating anymore

https://codereview.chromium.org/1403083002/diff/100001/blimp/common/proto/con...
blimp/common/proto/control.proto:7: // Current definitions are just placeholders
and are NOT final.
Remove these lines

https://codereview.chromium.org/1403083002/diff/100001/blimp/common/proto/con...
blimp/common/proto/control.proto:30: // When the type is LOAD_URL.
Not necessary; naming convention covers us there.

https://codereview.chromium.org/1403083002/diff/100001/blimp/engine/browser/b...
File blimp/engine/browser/blimp_engine_session.cc (right):

https://codereview.chromium.org/1403083002/diff/100001/blimp/engine/browser/b...
blimp/engine/browser/blimp_engine_session.cc:81: if (message.type() !=
BlimpMessage::CONTROL)
DCHECK(message.type() != BlimpMessage::CONTROL)

If the Dispatcher is doing its job, you don't need this.

https://codereview.chromium.org/1403083002/diff/100001/blimp/engine/browser/b...
File blimp/engine/browser/blimp_engine_session.h (right):

https://codereview.chromium.org/1403083002/diff/100001/blimp/engine/browser/b...
blimp/engine/browser/blimp_engine_session.h:74: // only one web_contents is
supported for blimp 0.5
Capitalize "Only"

[haibinlu, 2015-10-15 22:12:32]

https://codereview.chromium.org/1403083002/diff/100001/blimp/common/proto/con...
File blimp/common/proto/control.proto (right):

https://codereview.chromium.org/1403083002/diff/100001/blimp/common/proto/con...
blimp/common/proto/control.proto:5: // Message definitions for
client-originating browser control messages.
On 2015/10/15 22:01:56, Kevin M wrote:
> Not client-originating anymore

Done.

https://codereview.chromium.org/1403083002/diff/100001/blimp/common/proto/con...
blimp/common/proto/control.proto:7: // Current definitions are just placeholders
and are NOT final.
On 2015/10/15 22:01:56, Kevin M wrote:
> Remove these lines

Done.

https://codereview.chromium.org/1403083002/diff/100001/blimp/common/proto/con...
blimp/common/proto/control.proto:30: // When the type is LOAD_URL.
On 2015/10/15 22:01:56, Kevin M wrote:
> Not necessary; naming convention covers us there.

Done.

https://codereview.chromium.org/1403083002/diff/100001/blimp/engine/browser/b...
File blimp/engine/browser/blimp_engine_session.cc (right):

https://codereview.chromium.org/1403083002/diff/100001/blimp/engine/browser/b...
blimp/engine/browser/blimp_engine_session.cc:81: if (message.type() !=
BlimpMessage::CONTROL)
On 2015/10/15 22:01:56, Kevin M wrote:
> DCHECK(message.type() != BlimpMessage::CONTROL)
> 
> If the Dispatcher is doing its job, you don't need this.

Done.

https://codereview.chromium.org/1403083002/diff/100001/blimp/engine/browser/b...
File blimp/engine/browser/blimp_engine_session.h (right):

https://codereview.chromium.org/1403083002/diff/100001/blimp/engine/browser/b...
blimp/engine/browser/blimp_engine_session.h:74: // only one web_contents is
supported for blimp 0.5
On 2015/10/15 22:01:56, Kevin M wrote:
> Capitalize "Only"

Done.

[haibinlu, 2015-10-15 22:12:54]

The CQ bit was checked by haibinlu@chromium.org

[haibinlu, 2015-10-15 22:12:54]

The patchset sent to the CQ was uploaded after l-g-t-m from
kmarshall@chromium.org
Link to the patchset: https://codereview.chromium.org/1403083002/#ps120001
(title: " ")

[commit-bot: I haz the power, 2015-10-15 22:13:27]

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

[commit-bot: I haz the power, 2015-10-15 22:56:54]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-10-15 22:57:30]

Patchset 7 (id:??) landed as
https://crrev.com/28516c7ad6686f548c1c7f971b62c5f359f37b53
Cr-Commit-Position: refs/heads/master@{#354388}