mandoline filesystem: Save cookie data to the mojo:filesystem.

mandoline filesystem: Reland "Save cookie data to the mojo:filesystem."

In addition to saving the cookies to the file system service (per the
previous patch), we also modify the filesystem service to take a
client. When the shell connection to the file system service is broken,
instead of quitting immediately, we broadcast a shutdown notification to
all of our clients, and wait for them to close the directory objects.

This solves the timing problems during shutdown.

BUG=493311
TBR=shess@chromium.org
R=jam@chromium.org,erikwright@chromium.org
First Review URL: https://codereview.chromium.org/1179413010

Committed: https://crrev.com/e3ec7c45bd64ef826fd3940c8a2302f5b55edc45
Cr-Commit-Position: refs/heads/master@{#338888}

[Elliot Glaysher, 2015-07-10 17:45:33]

erg@chromium.org changed reviewers:
+ jam@chromium.org, shess@google.com

[Elliot Glaysher, 2015-07-10 17:45:34]

jam: Patch 1 was the original patch, patchset 2 are the modifications that
control lifetime.

shess: TBR. The only difference in in mojo_vfs.cc from the previous patch is
silencing a really spammy NOTREACHED().

[Elliot Glaysher, 2015-07-10 23:05:47]

ping

[jam, 2015-07-13 15:48:33]

lgtm

https://codereview.chromium.org/1231493002/diff/40001/components/filesystem/f...
File components/filesystem/file_system_app.h (right):

https://codereview.chromium.org/1231493002/diff/40001/components/filesystem/f...
components/filesystem/file_system_app.h:33: // actually take a
nit: rest of comment?

https://codereview.chromium.org/1231493002/diff/40001/mojo/application/public...
File mojo/application/public/cpp/application_delegate.h (right):

https://codereview.chromium.org/1231493002/diff/40001/mojo/application/public...
mojo/application/public/cpp/application_delegate.h:44: // down the connection,
but user is then required to call QuitNow() when done.
nit, to be clear, ApplicationImpl::QuitNow()?

https://codereview.chromium.org/1231493002/diff/40001/mojo/application/public...
mojo/application/public/cpp/application_delegate.h:45: virtual bool
OnShellConnectionError();
nit: perhaps the method name would make things obvious if it was called
QuitOnShellConnectionError?

https://codereview.chromium.org/1231493002/diff/40001/mojo/services/network/n...
File mojo/services/network/network_service_delegate.cc (right):

https://codereview.chromium.org/1231493002/diff/40001/mojo/services/network/n...
mojo/services/network/network_service_delegate.cc:181: // depending on the
connection an
nit: comment cut off

https://codereview.chromium.org/1231493002/diff/40001/mojo/services/network/n...
File mojo/services/network/network_service_delegate_observer.h (right):

https://codereview.chromium.org/1231493002/diff/40001/mojo/services/network/n...
mojo/services/network/network_service_delegate_observer.h:19: virtual void
OnIOWorkerThreadShutdown() = 0;
are we expecting more methods on this interface? if not, a callback seems
simpler..

https://codereview.chromium.org/1231493002/diff/40001/net/extras/sqlite/sqlit...
File net/extras/sqlite/sqlite_persistent_cookie_store.h (right):

https://codereview.chromium.org/1231493002/diff/40001/net/extras/sqlite/sqlit...
net/extras/sqlite/sqlite_persistent_cookie_store.h:53: void Close(const
base::Closure& callback);
please add an owner from net/extras/sqlite/OWNERS or net/OWNERS to review these
files in net/

[Elliot Glaysher, 2015-07-13 18:01:40]

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

[Elliot Glaysher, 2015-07-13 18:01:41]

erikwright: please review net/extras/sqlite/

The change I made here is a simple way to block on the Backend() shutdown.

https://codereview.chromium.org/1231493002/diff/40001/mojo/services/network/n...
File mojo/services/network/network_service_delegate_observer.h (right):

https://codereview.chromium.org/1231493002/diff/40001/mojo/services/network/n...
mojo/services/network/network_service_delegate_observer.h:19: virtual void
OnIOWorkerThreadShutdown() = 0;
On 2015/07/13 15:48:33, jam wrote:
> are we expecting more methods on this interface? if not, a callback seems
> simpler..

(talked about this in person; we need to keep track of observers to remove
them.)

[erikwright (departed), 2015-07-14 15:04:49]

https://codereview.chromium.org/1231493002/diff/60001/net/extras/sqlite/sqlit...
File net/extras/sqlite/sqlite_persistent_cookie_store.h (right):

https://codereview.chromium.org/1231493002/diff/60001/net/extras/sqlite/sqlit...
net/extras/sqlite/sqlite_persistent_cookie_store.h:52: // thread.
Add documentation about how this affects the PersistentCookieStore methods? In
particular, that the methods should fail and their callbacks should be invoked.

Test coverage of that, too.

[Elliot Glaysher, 2015-07-14 20:58:42]

Updated documentation, ignored calls once Close() was called, and added tests to
make sure that the two methods which had callbacks were called back
synchronously after Close().

[Elliot Glaysher, 2015-07-15 17:45:17]

ping erikwright

[erikwright (departed), 2015-07-15 17:52:04]

LGTM.

[Elliot Glaysher, 2015-07-15 17:58:49]

The CQ bit was checked by erg@chromium.org

[Elliot Glaysher, 2015-07-15 17:58:50]

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/1231493002/#ps100001
(title: "Merge with ToT")

[commit-bot: I haz the power, 2015-07-15 18:00:41]

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

[commit-bot: I haz the power, 2015-07-15 18:31:01]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-07-15 18:32:03]

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

[Scott Hess - ex-Googler, 2015-07-21 21:55:24]

shess@chromium.org changed reviewers:
+ shess@chromium.org

[Scott Hess - ex-Googler, 2015-07-21 21:55:24]

lgtm

https://codereview.chromium.org/1231493002/diff/100001/sql/mojo/mojo_vfs.cc
File sql/mojo/mojo_vfs.cc (right):

https://codereview.chromium.org/1231493002/diff/100001/sql/mojo/mojo_vfs.cc#n...
sql/mojo/mojo_vfs.cc:208: // so not leaving a NOTIMPLEMENTED().)
I think "return 0;" is a fine implementation of this.  Any other implementation
either needs to reflect per-platform characteristics or requires guarantees from
the implementation.

Reading the Windows and Unix implementations makes me think this probably makes
sense:
  return SQLITE_IOCAP_UNDELETABLE_WHEN_OPEN |
      SQLITE_IOCAP_POWERSAFE_OVERWRITE;
The first is technically not true for Unix, but I think that it will work fine.