Mandoline: Introduce ApplicationConnection::CloseConnection

Mandoline: Introduce ApplicationConnection::CloseConnection

Close the ApplicationConnection when ViewManagerInit is destroyed (the browser is closed).

BUG=496935
Committed: https://crrev.com/d8958d42cdf208d931f721a96cc31d826b54f86a
Cr-Commit-Position: refs/heads/master@{#336826}

[Fady Samuel, 2015-06-19 17:28:10]

fsamuel@chromium.org changed reviewers:
+ sky@chromium.org

[sky, 2015-06-19 19:22:20]

Please add test coverage too.

https://codereview.chromium.org/1195003002/diff/1/mojo/application/public/cpp...
File mojo/application/public/cpp/application_connection.h (right):

https://codereview.chromium.org/1195003002/diff/1/mojo/application/public/cpp...
mojo/application/public/cpp/application_connection.h:43: class
ApplicationConnection {
I would add a description of lifetime here too.

https://codereview.chromium.org/1195003002/diff/1/mojo/application/public/cpp...
File mojo/application/public/cpp/application_impl.h (right):

https://codereview.chromium.org/1195003002/diff/1/mojo/application/public/cpp...
mojo/application/public/cpp/application_impl.h:79: // Requests a new connection
to an application. Returns a pointer to the
Update this documentation.

https://codereview.chromium.org/1195003002/diff/1/mojo/application/public/cpp...
File mojo/application/public/cpp/lib/application_impl.cc (right):

https://codereview.chromium.org/1195003002/diff/1/mojo/application/public/cpp...
mojo/application/public/cpp/lib/application_impl.cc:87: delete *it;
IMO I would erase first, then delete. That way you don't have to worry about
side effects to the underlying vector.

https://codereview.chromium.org/1195003002/diff/1/mojo/application/public/cpp...
mojo/application/public/cpp/lib/application_impl.cc:88:
outgoing_service_registries_.erase(it);
Can't this be used for incoming connections too?

[Fady Samuel, 2015-06-26 22:41:20]

PTAL Addressed comments + Added test.

Introduced BrowserDelegate that is overridden in test to replace BrowserManager.

I suspect Browser Apptest will be more useful as multiwindow support progresses.

https://codereview.chromium.org/1195003002/diff/1/mojo/application/public/cpp...
File mojo/application/public/cpp/application_connection.h (right):

https://codereview.chromium.org/1195003002/diff/1/mojo/application/public/cpp...
mojo/application/public/cpp/application_connection.h:43: class
ApplicationConnection {
On 2015/06/19 19:22:19, sky wrote:
> I would add a description of lifetime here too.

Done.

https://codereview.chromium.org/1195003002/diff/1/mojo/application/public/cpp...
File mojo/application/public/cpp/application_impl.h (right):

https://codereview.chromium.org/1195003002/diff/1/mojo/application/public/cpp...
mojo/application/public/cpp/application_impl.h:79: // Requests a new connection
to an application. Returns a pointer to the
On 2015/06/19 19:22:19, sky wrote:
> Update this documentation.

Done.

https://codereview.chromium.org/1195003002/diff/1/mojo/application/public/cpp...
File mojo/application/public/cpp/lib/application_impl.cc (right):

https://codereview.chromium.org/1195003002/diff/1/mojo/application/public/cpp...
mojo/application/public/cpp/lib/application_impl.cc:87: delete *it;
On 2015/06/19 19:22:19, sky wrote:
> IMO I would erase first, then delete. That way you don't have to worry about
> side effects to the underlying vector.

Done.

https://codereview.chromium.org/1195003002/diff/1/mojo/application/public/cpp...
mojo/application/public/cpp/lib/application_impl.cc:88:
outgoing_service_registries_.erase(it);
On 2015/06/19 19:22:19, sky wrote:
> Can't this be used for incoming connections too?

Done.

[sky, 2015-06-29 16:56:02]

I was hoping for a more applicationconnection specific test. Is it possible to
write one in mojo/application/public/cpp/tests?

https://codereview.chromium.org/1195003002/diff/20001/mandoline/ui/browser/br...
File mandoline/ui/browser/browser.h (right):

https://codereview.chromium.org/1195003002/diff/20001/mandoline/ui/browser/br...
mandoline/ui/browser/browser.h:50: mojo::ApplicationConnection*
GetViewManagerConnectionForTesting();
I'm not a fan of public functions for testing. I prefer friends. They don't
clutter the API.

https://codereview.chromium.org/1195003002/diff/20001/mandoline/ui/browser/br...
File mandoline/ui/browser/browser_apptest.cc (right):

https://codereview.chromium.org/1195003002/diff/20001/mandoline/ui/browser/br...
mandoline/ui/browser/browser_apptest.cc:5: #include
"mandoline/ui/browser/browser.h"
nit: newline after this line.

https://codereview.chromium.org/1195003002/diff/20001/mandoline/ui/browser/br...
mandoline/ui/browser/browser_apptest.cc:22: DCHECK(app_);
If you can, avoid DCHECKs in tests. They trigger a crash of the whole harness.
Instead use ASSERT/EXPECT/FAIL... In this particular case I would early return
nullptr, the call site ASSERTs on the return value, so that should be fine.

https://codereview.chromium.org/1195003002/diff/20001/mandoline/ui/browser/br...
File mandoline/ui/browser/browser_delegate.h (right):

https://codereview.chromium.org/1195003002/diff/20001/mandoline/ui/browser/br...
mandoline/ui/browser/browser_delegate.h:24: // Requests initialization of state
to display the browser on screen.
Document return value.

https://codereview.chromium.org/1195003002/diff/20001/mandoline/ui/browser/br...
mandoline/ui/browser/browser_delegate.h:26: protected:
nit: newline between 25/26.

https://codereview.chromium.org/1195003002/diff/20001/mojo/application/public...
File mojo/application/public/cpp/application_connection.h (right):

https://codereview.chromium.org/1195003002/diff/20001/mojo/application/public...
mojo/application/public/cpp/application_connection.h:48: virtual
~ApplicationConnection();
Can the destructor be made protected to prevent accidental usage? Alternatively
can you make it so deleting the connection closes the connection?

[Fady Samuel, 2015-06-29 20:32:36]

PTAL service_registry_unittest seems to be the ApplicationConnection test you're
looking for. By making the destructor private, that forces it to go through
CloseConnection which gets us the test coverage.

https://codereview.chromium.org/1195003002/diff/20001/mandoline/ui/browser/br...
File mandoline/ui/browser/browser.h (right):

https://codereview.chromium.org/1195003002/diff/20001/mandoline/ui/browser/br...
mandoline/ui/browser/browser.h:50: mojo::ApplicationConnection*
GetViewManagerConnectionForTesting();
On 2015/06/29 16:56:02, sky wrote:
> I'm not a fan of public functions for testing. I prefer friends. They don't
> clutter the API.

Done.

https://codereview.chromium.org/1195003002/diff/20001/mandoline/ui/browser/br...
File mandoline/ui/browser/browser_apptest.cc (right):

https://codereview.chromium.org/1195003002/diff/20001/mandoline/ui/browser/br...
mandoline/ui/browser/browser_apptest.cc:5: #include
"mandoline/ui/browser/browser.h"
On 2015/06/29 16:56:02, sky wrote:
> nit: newline after this line.

Done.

https://codereview.chromium.org/1195003002/diff/20001/mandoline/ui/browser/br...
mandoline/ui/browser/browser_apptest.cc:22: DCHECK(app_);
On 2015/06/29 16:56:02, sky wrote:
> If you can, avoid DCHECKs in tests. They trigger a crash of the whole harness.
> Instead use ASSERT/EXPECT/FAIL... In this particular case I would early return
> nullptr, the call site ASSERTs on the return value, so that should be fine.

Done.

https://codereview.chromium.org/1195003002/diff/20001/mandoline/ui/browser/br...
File mandoline/ui/browser/browser_delegate.h (right):

https://codereview.chromium.org/1195003002/diff/20001/mandoline/ui/browser/br...
mandoline/ui/browser/browser_delegate.h:24: // Requests initialization of state
to display the browser on screen.
On 2015/06/29 16:56:02, sky wrote:
> Document return value.

Done.

https://codereview.chromium.org/1195003002/diff/20001/mandoline/ui/browser/br...
mandoline/ui/browser/browser_delegate.h:26: protected:
On 2015/06/29 16:56:02, sky wrote:
> nit: newline between 25/26.

Done.

https://codereview.chromium.org/1195003002/diff/20001/mojo/application/public...
File mojo/application/public/cpp/application_connection.h (right):

https://codereview.chromium.org/1195003002/diff/20001/mojo/application/public...
mojo/application/public/cpp/application_connection.h:48: virtual
~ApplicationConnection();
On 2015/06/29 16:56:02, sky wrote:
> Can the destructor be made protected to prevent accidental usage?
Alternatively
> can you make it so deleting the connection closes the connection?

I took the following approach.

1. I made ~ApplicationConnection and ~ServiceRegistry protected.
2. I made CloseConnection to be non-virtual.
3. I introduced a protected pure virtual OnCloseConnection() that is overridden
by ServiceRegistry.
4. I introduced a flag connection_closed_ in ApplicationConnection that becomes
true when CloseConnection is called. This both prevents double deletes and
ensures that only CloseConnection can actually destroy the object.
5. I added a DCHECK in ~ApplicationConnection to verify that the
connection_closed_ flag is set.

[sky, 2015-06-29 21:26:00]

https://codereview.chromium.org/1195003002/diff/80001/mojo/application/public...
File mojo/application/public/cpp/lib/application_impl.cc (right):

https://codereview.chromium.org/1195003002/diff/80001/mojo/application/public...
mojo/application/public/cpp/lib/application_impl.cc:85:
delegate_->OnWillCloseConnection(connection);
Is this problematic for HTMLViewer? HTMLViewer has an ApplicationImpl as a
member. The member is destroyed after HTMLViewers destructor completes. Maybe
HTMLViewer shouldn't have it by value and instead wrap the ApplicationImpl in a
scoped_ptr and explicitly destroy early on?

[Fady Samuel, 2015-06-29 21:50:15]

PTAL

https://codereview.chromium.org/1195003002/diff/80001/mojo/application/public...
File mojo/application/public/cpp/lib/application_impl.cc (right):

https://codereview.chromium.org/1195003002/diff/80001/mojo/application/public...
mojo/application/public/cpp/lib/application_impl.cc:85:
delegate_->OnWillCloseConnection(connection);
On 2015/06/29 21:26:00, sky wrote:
> Is this problematic for HTMLViewer? HTMLViewer has an ApplicationImpl as a
> member. The member is destroyed after HTMLViewers destructor completes. Maybe
> HTMLViewer shouldn't have it by value and instead wrap the ApplicationImpl in
a
> scoped_ptr and explicitly destroy early on?

This shouldn't be a problem. In ApplicationRunner, before I destroy the
ApplicationDelegate, I call ApplicationImpl::WillDestroySoon, which clears the
delegate_.

[sky, 2015-06-29 23:38:38]

https://codereview.chromium.org/1195003002/diff/80001/mojo/application/public...
File mojo/application/public/cpp/lib/application_connection.cc (right):

https://codereview.chromium.org/1195003002/diff/80001/mojo/application/public...
mojo/application/public/cpp/lib/application_connection.cc:14:
ApplicationConnection::~ApplicationConnection() {
nit: move to match position in header.

https://codereview.chromium.org/1195003002/diff/80001/mojo/application/public...
File mojo/application/public/cpp/lib/application_impl.cc (right):

https://codereview.chromium.org/1195003002/diff/80001/mojo/application/public...
mojo/application/public/cpp/lib/application_impl.cc:85:
delegate_->OnWillCloseConnection(connection);
On 2015/06/29 21:50:14, Fady Samuel wrote:
> On 2015/06/29 21:26:00, sky wrote:
> > Is this problematic for HTMLViewer? HTMLViewer has an ApplicationImpl as a
> > member. The member is destroyed after HTMLViewers destructor completes.
Maybe
> > HTMLViewer shouldn't have it by value and instead wrap the ApplicationImpl
in
> a
> > scoped_ptr and explicitly destroy early on?
> 
> This shouldn't be a problem. In ApplicationRunner, before I destroy the
> ApplicationDelegate, I call ApplicationImpl::WillDestroySoon, which clears the
> delegate_.

Could WillDestroySoon be replaced with a boolean member that indicates in
destructor? That is, ~ApplicationImpl sets in_destructor_ before doing anything
and CloseConnection() only notifies the delegate_ if !in_destructor_?

[Fady Samuel, 2015-06-30 15:07:22]

PTAL Scott! Thanks!

https://codereview.chromium.org/1195003002/diff/80001/mojo/application/public...
File mojo/application/public/cpp/lib/application_connection.cc (right):

https://codereview.chromium.org/1195003002/diff/80001/mojo/application/public...
mojo/application/public/cpp/lib/application_connection.cc:14:
ApplicationConnection::~ApplicationConnection() {
On 2015/06/29 23:38:38, sky wrote:
> nit: move to match position in header.

Done.

https://codereview.chromium.org/1195003002/diff/80001/mojo/application/public...
File mojo/application/public/cpp/lib/application_impl.cc (right):

https://codereview.chromium.org/1195003002/diff/80001/mojo/application/public...
mojo/application/public/cpp/lib/application_impl.cc:85:
delegate_->OnWillCloseConnection(connection);
On 2015/06/29 23:38:38, sky wrote:
> On 2015/06/29 21:50:14, Fady Samuel wrote:
> > On 2015/06/29 21:26:00, sky wrote:
> > > Is this problematic for HTMLViewer? HTMLViewer has an ApplicationImpl as a
> > > member. The member is destroyed after HTMLViewers destructor completes.
> Maybe
> > > HTMLViewer shouldn't have it by value and instead wrap the ApplicationImpl
> in
> > a
> > > scoped_ptr and explicitly destroy early on?
> > 
> > This shouldn't be a problem. In ApplicationRunner, before I destroy the
> > ApplicationDelegate, I call ApplicationImpl::WillDestroySoon, which clears
the
> > delegate_.
> 
> Could WillDestroySoon be replaced with a boolean member that indicates in
> destructor? That is, ~ApplicationImpl sets in_destructor_ before doing
anything
> and CloseConnection() only notifies the delegate_ if !in_destructor_?

Yes, that works. Done.

[sky, 2015-06-30 16:41:47]

LGTM

https://codereview.chromium.org/1195003002/diff/100001/mandoline/ui/browser/b...
File mandoline/ui/browser/browser.cc (right):

https://codereview.chromium.org/1195003002/diff/100001/mandoline/ui/browser/b...
mandoline/ui/browser/browser.cc:72: mojo::ApplicationConnection*
Browser::GetViewManagerConnectionForTesting() {
Position should match that of header.

https://codereview.chromium.org/1195003002/diff/100001/mandoline/ui/browser/b...
File mandoline/ui/browser/browser_apptest.cc (right):

https://codereview.chromium.org/1195003002/diff/100001/mandoline/ui/browser/b...
mandoline/ui/browser/browser_apptest.cc:61: mojo::ApplicationConnection*
last_closed_connection_;
Can the type of last_closed_connection_ be a void* (as well as
last_closed_connection()) to make it more obvious it shouldn't be derefenced.

https://codereview.chromium.org/1195003002/diff/100001/mojo/application/publi...
File mojo/application/public/cpp/lib/service_registry.h (right):

https://codereview.chromium.org/1195003002/diff/100001/mojo/application/publi...
mojo/application/public/cpp/lib/service_registry.h:56: ~ServiceRegistry()
override;
Move definition to match new position of declaration.

[Fady Samuel, 2015-06-30 17:31:19]

Addressed nits. CQ'ing now.

https://codereview.chromium.org/1195003002/diff/100001/mandoline/ui/browser/b...
File mandoline/ui/browser/browser.cc (right):

https://codereview.chromium.org/1195003002/diff/100001/mandoline/ui/browser/b...
mandoline/ui/browser/browser.cc:72: mojo::ApplicationConnection*
Browser::GetViewManagerConnectionForTesting() {
On 2015/06/30 16:41:46, sky wrote:
> Position should match that of header.

Done.

https://codereview.chromium.org/1195003002/diff/100001/mandoline/ui/browser/b...
File mandoline/ui/browser/browser_apptest.cc (right):

https://codereview.chromium.org/1195003002/diff/100001/mandoline/ui/browser/b...
mandoline/ui/browser/browser_apptest.cc:61: mojo::ApplicationConnection*
last_closed_connection_;
On 2015/06/30 16:41:46, sky wrote:
> Can the type of last_closed_connection_ be a void* (as well as
> last_closed_connection()) to make it more obvious it shouldn't be derefenced.

Done.

https://codereview.chromium.org/1195003002/diff/100001/mojo/application/publi...
File mojo/application/public/cpp/lib/service_registry.h (right):

https://codereview.chromium.org/1195003002/diff/100001/mojo/application/publi...
mojo/application/public/cpp/lib/service_registry.h:56: ~ServiceRegistry()
override;
On 2015/06/30 16:41:46, sky wrote:
> Move definition to match new position of declaration.

Done.

[Fady Samuel, 2015-06-30 17:31:24]

The CQ bit was checked by fsamuel@chromium.org

[Fady Samuel, 2015-06-30 17:31:24]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1195003002/#ps120001
(title: "Addressed sky's nits")

[commit-bot: I haz the power, 2015-06-30 17:33:14]

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

[commit-bot: I haz the power, 2015-06-30 18:42:03]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-06-30 18:44:15]

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