Add support for loading pak files from arbitrary file regions.

base/files/file.h

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef BASE_FILES_FILE_H_
 #define BASE_FILES_FILE_H_
 
 #include "build/build_config.h"
 #if defined(OS_WIN)
 #include <windows.h>
@@ skipping 137 matching lines @@
     // The last modified time of a file.
     base::Time last_modified;
 
     // The last accessed time of a file.
     base::Time last_accessed;
 
     // The creation time of a file.
     base::Time creation_time;
   };
 
+  // Used to hold information about a region [offset + size] of a file.
+  struct BASE_EXPORT Region {

[willchan no longer on Chromium, 2014/07/21 23:55:40]

Why is this a property of the file as opposed to the memory map? Unless there
are other consumers that are interested in file regions (very possible! but I
don't know of any yet), I'd prefer to keep this struct in MemoryMappedFile
instead.

[willchan no longer on Chromium, 2014/07/22 20:45:47]

Can you address this comment?

[Primiano Tucci (use gerrit), 2014/07/23 23:17:38]

I'm on the fence on this.
On your side: yes, the reason why I introduce the Region concept is essentially
for mmap.

On the other side: look at resource_bundle.cc (which is out of /base) in this CL
(lines 238-242):
It does something like this:
void ResourceBundle::AddDataPackFromFile(base::File, ScaleFactor scale_factor) {
  AddDataPackFromFileRegion(file.Pass(), base::File::Region::kWholeFile, ...);

ResourceBundle today already knows the concept of a File. ResourceBundle,
however, is oblivious of the fact that DataPack (a class it uses) will do a mmap
operation on that File (RB just passes the File along).
If i keep the Region as a File-related concept, ResourceBundle will keep to be
oblivious about the mmap implementation detail of DataPack.
If I move Region to memory_mapped_file.h, now the ResourceBundle (and DataPack)
method signatures will expose this further detail. Essentially the final
signature of DataPack would become:

AddDataPackFromFileRegion(base::File, MemoryMappedFile::Region), which IMHO is a
bit ackward
vs the current
AddDataPackFromFileRegion(base::File, base::File::Region), which IMHO looks
better.

What do you think?

[willchan no longer on Chromium, 2014/07/25 21:21:23]

I think that you raise an interesting point that might qualify as another
consumer that is interested in file regions. I'm still mulling it over.

I'm trying to understand the layering with ResourceBundle. Is ResourceBundle
supposed to understand the backing store for a DataPack or not? If not, why
doesn't it expose ResourceBundle::AddDataPack() and allow the embedder to pass
it a DataPack with whatever backing store it wants? If ResourceBundle is
supposed to understand the backing store, why is it OK to understand files but
not memory mapped files?

I'm guessing this is a question for Tony?

I guess the primary point is that it might be useful to use a file backed
ResourceBundle, but not necessarily a memory mapped filed backed ResourceBundle.
But would this ever actually be useful? If not, it's less mental comprehension
overhead just to cut to the chase and only expose the memory mapped one.

I'm open to keeping this in File. If this abstraction layer is useful, let's do
it. But I'm not immediately convinced of its utility.

[Primiano Tucci (use gerrit), 2014/07/28 12:52:11]

I was looking again at this code with fresh mind, after the weekend, and I think
that:
It is fine that DataPack/ResourceBundle expose the data store (i.e. the file),
as that is part of the service they exhibit: they're offering the possibility of
loading stuff from a file, so that's fine that their contract knows about
base::File.
However, if Region becomes part of MemMapFile, DataPack/ResourceBundle would
start exposing unnecessary details of their internal implementation (the fact
that they are mmap()-ing reather than read()-ing), which IHMO looks less nice.

In other words, what if tomorrow DP/RB want to just read() the region from the
file, for instance because they need to decode a protobuf from that and don't
need to keep the memory around?
The more I look at that, the more it feels to me that Region itself is a concept
more related to a File.

Anyways, I'm going to prepare a 2nd CL which is the exact copy of this, with the
only exception of Region being moved to MemoryMappedFile.
You guys have the final word, I'm fine either ways.
I'd just kindly ask you to LGTM at this point the one you finally like the most
(provided that there are no other concerns left) and let me and save another day
of cross-timezone reviewing.

[willchan no longer on Chromium, 2014/07/28 14:52:16]

Hm, well this require refactoring all uses today or introducing a new, separate
API method, right? The existing ResourceBundle API makes the assumption that the
file region encoding already matches what it'd be in memory. You can't support a
new encoding in the current API.

That said, I did acknowledge something similar to this above:

"""
I guess the primary point is that it might be useful to use a file
backed ResourceBundle, but not necessarily a memory mapped filed backed
ResourceBundle. But would this ever actually be useful? If not, it's
less mental comprehension overhead just to cut to the chase and only
expose the memory mapped one.
"""

Generally, for base/ stuff, we err on the side of not introducing new
abstractions in the *possibility* of future use. We wait until the new use
actually happens. We'd let it in only use we knew it was going to be used in a
followup CL.

Furthermore, I'd assert that if ResourceBundle were going to introduce this kind
of flexibility in the future, it should just take in the DataPack rather than
have all the input parameters for the DataPack's construction in the
ResourceBundle's API. ResourceBundle doesn't need to know how the DataPack's are
constructed. DataPacks could be constructed with the appropriate backend (memory
map, protobuffer, etc) and passed in post-construction to the ResourceBundle.

+    static Region WholeFile();
+
+    Region(int64 offset, int64 size);
+    ~Region();
+
+    // Whether the region intends to cover the entire file.

[willchan no longer on Chromium, 2014/07/21 23:55:40]

What's the intended behavior when a file is mapped using regions for the whole
file, but afterward, the file is appended to. Should IsWholeFile() still return
true? This should be documented and a test should cover this.

[Primiano Tucci (use gerrit), 2014/07/23 23:17:38]

I think we discussed this offline and kWholeFile should remove any ambiguity.
So...*puff* this method has gone. Together with it, also the empty ~dtor has
disappeared. Rationale: even if it's empty, the static kWholeFile would have
created an exit time destructor (or at least, clang's -Wexit-time-destructors
strongly believed that, firing an error, which went away removing the dtor).  

+    bool IsWholeFile() const;
+
+    // Start of the region (measured in bytes from the beginning of the file).
+    int64 offset;
+
+    // Length of the region.
+    int64 size;
+  };
+
   File();
 
   // Creates or opens the given file. This will fail with 'access denied' if the
   // |name| contains path traversal ('..') components.
   File(const FilePath& name, uint32 flags);
 
   // Takes ownership of |platform_file|.
   explicit File(PlatformFile platform_file);
 
   // Creates an object with a specific error_details code.
@@ skipping 136 matching lines @@
 #endif
 
   Error error_details_;
   bool created_;
   bool async_;
 };
 
 }  // namespace base
 
 #endif  // BASE_FILES_FILE_H_