Docserver: make file_system a property of Create

Docserver: Make the file_system argument to CompiledFileSystem be a property of
CFS.Create rather than its constructor. This will allow ad-hoc construction of CFSs,
essential for AvailabilityFinder to be able to create them per-branch to address cron
slowness. It will also allow the creation of other CFS-like entities, such as a "JsonFS".

BUG=305280, 295812
R=jyasskin@chromium.org
NOTRY=true

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=228533

[Jeffrey Yasskin, 2013-10-14 18:52:30]

https://codereview.chromium.org/26538009/diff/6001/chrome/common/extensions/d...
File chrome/common/extensions/docs/server2/caching_file_system.py (right):

https://codereview.chromium.org/26538009/diff/6001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/caching_file_system.py:127: return ' of
%s' % self._file_system
Is there something missing before ' of'? If not, please comment why.

https://codereview.chromium.org/26538009/diff/6001/chrome/common/extensions/d...
File chrome/common/extensions/docs/server2/chained_compiled_file_system.py
(right):

https://codereview.chromium.org/26538009/diff/6001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/chained_compiled_file_system.py:35: for fs
in [file_system] + self._file_system_chain))
This seems a little odd. Why do we want to pass the chain of filesystems in two
different places? Does the first element vary while the rest are constant, in
actual uses? If so, comment that. If this is an interface-matching thing,
comment that. Otherwise, I think you should pass the whole thing in either the
Factory constructor or the Create() function.

https://codereview.chromium.org/26538009/diff/6001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/chained_compiled_file_system.py:72:
first_version = first_compiled_fs._file_system.Stat(path).version
Hm, accessing a protected field is a yellow flag: does it make sense to make
this a public accessor?

https://codereview.chromium.org/26538009/diff/6001/chrome/common/extensions/d...
File chrome/common/extensions/docs/server2/compiled_file_system.py (right):

https://codereview.chromium.org/26538009/diff/6001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/compiled_file_system.py:25: def
Create(self, file_system, populate_function, cls, category=None):
Please document the file_system argument.

https://codereview.chromium.org/26538009/diff/6001/chrome/common/extensions/d...
File chrome/common/extensions/docs/server2/compiled_file_system_test.py (right):

https://codereview.chromium.org/26538009/diff/6001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/compiled_file_system_test.py:42: def
_CreateTestObjects():
Instead of returning an obscure tuple, how about returning the Create() function
with 'fs' bound in:

def _CreateFactory():
  factory = CompiledFileSystem.Factory(
           ObjectStoreCreator(start_empty=False,
                              store_type=TestObjectStore,
                              disable_wrappers=True))
  return functools.partial(factory.Create, TestFileSystem(deepcopy(_TEST_DATA)))

https://codereview.chromium.org/26538009/diff/6001/chrome/common/extensions/d...
File chrome/common/extensions/docs/server2/host_file_system_provider.py (right):

https://codereview.chromium.org/26538009/diff/6001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/host_file_system_provider.py:98:
optargs['constructor_for_test'] = lambda **_: LocalFileSystem.Create()
You can write:

HostFileSystemProvider(
  object_store_creator,
  constructor_for_test=lambda **_: LocalFileSystem.Create(),
  **optargs)

That'll throw an exception if the user passes 'constructor_for_test' into
ForLocal, while yours will silently override their argument, but I suspect that
difference doesn't matter.

https://codereview.chromium.org/26538009/diff/6001/chrome/common/extensions/d...
File chrome/common/extensions/docs/server2/test_file_system.py (right):

https://codereview.chromium.org/26538009/diff/6001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/test_file_system.py:110: def
IncrementStat(self, path=None):
Should this become IncrementVersion at some point?

[not at google - send to devlin, 2013-10-14 21:03:33]

https://codereview.chromium.org/26538009/diff/6001/chrome/common/extensions/d...
File chrome/common/extensions/docs/server2/caching_file_system.py (right):

https://codereview.chromium.org/26538009/diff/6001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/caching_file_system.py:127: return ' of
%s' % self._file_system
On 2013/10/14 18:52:31, Jeffrey Yasskin wrote:
> Is there something missing before ' of'? If not, please comment why.

GetDebugString is weird in that FileSystem implement __repr__ and __str__ in
terms of it, but always inserts the type beforehand. Come to think of it that's
weird and I'll just make it properly overridable.

https://codereview.chromium.org/26538009/diff/6001/chrome/common/extensions/d...
File chrome/common/extensions/docs/server2/chained_compiled_file_system.py
(right):

https://codereview.chromium.org/26538009/diff/6001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/chained_compiled_file_system.py:35: for fs
in [file_system] + self._file_system_chain))
On 2013/10/14 18:52:31, Jeffrey Yasskin wrote:
> This seems a little odd. Why do we want to pass the chain of filesystems in
two
> different places? Does the first element vary while the rest are constant, in
> actual uses? If so, comment that. If this is an interface-matching thing,
> comment that. Otherwise, I think you should pass the whole thing in either the
> Factory constructor or the Create() function.

I'll clarify this in the docstring. Updated some of the other comments in this
file too.

https://codereview.chromium.org/26538009/diff/6001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/chained_compiled_file_system.py:72:
first_version = first_compiled_fs._file_system.Stat(path).version
On 2013/10/14 18:52:31, Jeffrey Yasskin wrote:
> Hm, accessing a protected field is a yellow flag: does it make sense to make
> this a public accessor?

I swear I changed this to use version_getter(..) before. Oops. Thanks for
catching.

https://codereview.chromium.org/26538009/diff/6001/chrome/common/extensions/d...
File chrome/common/extensions/docs/server2/compiled_file_system.py (right):

https://codereview.chromium.org/26538009/diff/6001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/compiled_file_system.py:25: def
Create(self, file_system, populate_function, cls, category=None):
On 2013/10/14 18:52:31, Jeffrey Yasskin wrote:
> Please document the file_system argument.

Done.

https://codereview.chromium.org/26538009/diff/6001/chrome/common/extensions/d...
File chrome/common/extensions/docs/server2/compiled_file_system_test.py (right):

https://codereview.chromium.org/26538009/diff/6001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/compiled_file_system_test.py:42: def
_CreateTestObjects():
On 2013/10/14 18:52:31, Jeffrey Yasskin wrote:
> Instead of returning an obscure tuple, how about returning the Create()
function
> with 'fs' bound in:
> 
> def _CreateFactory():
>   factory = CompiledFileSystem.Factory(
>            ObjectStoreCreator(start_empty=False,
>                               store_type=TestObjectStore,
>                               disable_wrappers=True))
>   return functools.partial(factory.Create,
TestFileSystem(deepcopy(_TEST_DATA)))

Done.

https://codereview.chromium.org/26538009/diff/6001/chrome/common/extensions/d...
File chrome/common/extensions/docs/server2/host_file_system_provider.py (right):

https://codereview.chromium.org/26538009/diff/6001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/host_file_system_provider.py:98:
optargs['constructor_for_test'] = lambda **_: LocalFileSystem.Create()
On 2013/10/14 18:52:31, Jeffrey Yasskin wrote:
> You can write:
> 
> HostFileSystemProvider(
>   object_store_creator,
>   constructor_for_test=lambda **_: LocalFileSystem.Create(),
>   **optargs)
> 
> That'll throw an exception if the user passes 'constructor_for_test' into
> ForLocal, while yours will silently override their argument, but I suspect
that
> difference doesn't matter.

Done.

https://codereview.chromium.org/26538009/diff/6001/chrome/common/extensions/d...
File chrome/common/extensions/docs/server2/test_file_system.py (right):

https://codereview.chromium.org/26538009/diff/6001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/test_file_system.py:110: def
IncrementStat(self, path=None):
On 2013/10/14 18:52:31, Jeffrey Yasskin wrote:
> Should this become IncrementVersion at some point?

I expect so, yes.

[Jeffrey Yasskin, 2013-10-14 21:24:17]

lgtm

https://codereview.chromium.org/26538009/diff/17001/chrome/common/extensions/...
File chrome/common/extensions/docs/server2/caching_file_system.py (right):

https://codereview.chromium.org/26538009/diff/17001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/caching_file_system.py:127: return ' of
%s' % self._file_system
Did you mean to remove GetDebugString() now that nothing uses it?

https://codereview.chromium.org/26538009/diff/17001/chrome/common/extensions/...
File chrome/common/extensions/docs/server2/chained_compiled_file_system.py
(right):

https://codereview.chromium.org/26538009/diff/17001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/chained_compiled_file_system.py:17: and
the chain the FileSystem which is patches, but with file systems
s/is/it/?

[not at google - send to devlin, 2013-10-14 21:26:27]

https://codereview.chromium.org/26538009/diff/17001/chrome/common/extensions/...
File chrome/common/extensions/docs/server2/caching_file_system.py (right):

https://codereview.chromium.org/26538009/diff/17001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/caching_file_system.py:127: return ' of
%s' % self._file_system
On 2013/10/14 21:24:17, Jeffrey Yasskin wrote:
> Did you mean to remove GetDebugString() now that nothing uses it?

yes, thanks. Done.

https://codereview.chromium.org/26538009/diff/17001/chrome/common/extensions/...
File chrome/common/extensions/docs/server2/chained_compiled_file_system.py
(right):

https://codereview.chromium.org/26538009/diff/17001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/chained_compiled_file_system.py:17: and
the chain the FileSystem which is patches, but with file systems
On 2013/10/14 21:24:17, Jeffrey Yasskin wrote:
> s/is/it/?

Done.

[not at google - send to devlin, 2013-10-14 21:32:08]

Committed patchset #5 manually as r228533 (presubmit successful).