AppCache INTERCEPT namespace.

AppCache INTERCEPT namespace.
- Add support for a custom CHROMIUM CACHE MANIFEST signature. Other browsers should ignore files with this signature.
- Parse intercept namespace entries out of the new CHROMIUM-INTERCEPT manifest section. Other browsers should ignore this entire section.
- Store and retrieve the new kind of namespace records in the database layer.
- Upgrade existing databases to the new schema.
- Look for matches in the new namespaces when handling main and sub resource requests.
- Add unit tests.

BUG=101565
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=114151

[michaeln, 2011-12-02 00:23:30]

hi jenn, this is quite ready for review yet but its getting close and i'm
wondering if you'd be a will reviewer?

[jennb, 2011-12-05 23:23:07]

Overall, looks good so far. Looks like most of this is "renaming" of the
fallback namespace vector to a more general namespace vector.

http://codereview.chromium.org/8396013/diff/32026/webkit/appcache/appcache_da...
File webkit/appcache/appcache_database_unittest.cc (right):

http://codereview.chromium.org/8396013/diff/32026/webkit/appcache/appcache_da...
webkit/appcache/appcache_database_unittest.cc:345:
std::vector<AppCacheDatabase::NamespaceRecord> records;
Rename to fallbacks?

http://codereview.chromium.org/8396013/diff/32026/webkit/appcache/appcache_in...
File webkit/appcache/appcache_interfaces.h (right):

http://codereview.chromium.org/8396013/diff/32026/webkit/appcache/appcache_in...
webkit/appcache/appcache_interfaces.h:98: Namespace();
Should comment that default is Fallback namespace if type not specified?

http://codereview.chromium.org/8396013/diff/32026/webkit/appcache/appcache_st...
File webkit/appcache/appcache_storage_impl.cc (right):

http://codereview.chromium.org/8396013/diff/32026/webkit/appcache/appcache_st...
webkit/appcache/appcache_storage_impl.cc:929: NetworkNamespaceHelper
network_namespace_helper(database_);
Intentional that network_namespace_helper isn't actually used?

http://codereview.chromium.org/8396013/diff/32026/webkit/appcache/appcache_st...
webkit/appcache/appcache_storage_impl.cc:1001: fallback_entry_ = AppCacheEntry(
Should this also be part of the if-fallback clause?

http://codereview.chromium.org/8396013/diff/32026/webkit/appcache/appcache_up...
File webkit/appcache/appcache_update_job_unittest.cc (right):

http://codereview.chromium.org/8396013/diff/32026/webkit/appcache/appcache_up...
webkit/appcache/appcache_update_job_unittest.cc:194: 
Extra blank line?

http://codereview.chromium.org/8396013/diff/32026/webkit/appcache/appcache_up...
webkit/appcache/appcache_update_job_unittest.cc:3054:
EXPECT_TRUE(cache->fallback_namespaces_[0] ==
Change to EXPECT_EQ?

http://codereview.chromium.org/8396013/diff/32026/webkit/appcache/appcache_up...
webkit/appcache/appcache_update_job_unittest.cc:3081:
EXPECT_TRUE(cache->fallback_namespaces_[0] ==
Ditto.

http://codereview.chromium.org/8396013/diff/32026/webkit/appcache/manifest_pa...
File webkit/appcache/manifest_parser_unittest.cc (right):

http://codereview.chromium.org/8396013/diff/32026/webkit/appcache/manifest_pa...
webkit/appcache/manifest_parser_unittest.cc:253: const std::string
kData("CHROMIUM CACHE MANIFEST\r"
Do we care if the manifest does not use the CHROMIUM signature, but still has an
INTERCEPT section? I'm not sure what's supposed to happen in that case.

http://codereview.chromium.org/8396013/diff/32026/webkit/appcache/manifest_pa...
webkit/appcache/manifest_parser_unittest.cc:255: "http://www.portme.com:1234/one
return relative/onefb\r"
the fb in onefb stands for fallback. ;-) 

Not that it matters enough to change for the intercept test.

[michaeln, 2011-12-06 02:07:05]

not updated yet, i'll ping you when i do and add more unit tests... thnx for
looking!

http://codereview.chromium.org/8396013/diff/32026/webkit/appcache/appcache_st...
File webkit/appcache/appcache_storage_impl.cc (right):

http://codereview.chromium.org/8396013/diff/32026/webkit/appcache/appcache_st...
webkit/appcache/appcache_storage_impl.cc:929: NetworkNamespaceHelper
network_namespace_helper(database_);
On 2011/12/05 23:23:07, jennb wrote:
> Intentional that network_namespace_helper isn't actually used?

It's passed to both FindNamespaceHelper() method calls below and used within
that method.

http://codereview.chromium.org/8396013/diff/32026/webkit/appcache/appcache_st...
webkit/appcache/appcache_storage_impl.cc:1001: fallback_entry_ = AppCacheEntry(
On 2011/12/05 23:23:07, jennb wrote:
> Should this also be part of the if-fallback clause?

Yes, thank you!

http://codereview.chromium.org/8396013/diff/32026/webkit/appcache/manifest_pa...
File webkit/appcache/manifest_parser_unittest.cc (right):

http://codereview.chromium.org/8396013/diff/32026/webkit/appcache/manifest_pa...
webkit/appcache/manifest_parser_unittest.cc:253: const std::string
kData("CHROMIUM CACHE MANIFEST\r"
On 2011/12/05 23:23:07, jennb wrote:
> Do we care if the manifest does not use the CHROMIUM signature, but still has
an
> INTERCEPT section? I'm not sure what's supposed to happen in that case.

I was going to allow the intercept namespaces to work in that case too.

http://codereview.chromium.org/8396013/diff/32026/webkit/appcache/manifest_pa...
webkit/appcache/manifest_parser_unittest.cc:255: "http://www.portme.com:1234/one
return relative/onefb\r"
On 2011/12/05 23:23:07, jennb wrote:
> the fb in onefb stands for fallback. ;-) 
> 
> Not that it matters enough to change for the intercept test.

oh... its not short for facebook :)

[michaeln, 2011-12-06 19:41:24]

still need new tests

http://codereview.chromium.org/8396013/diff/32026/webkit/appcache/appcache_da...
File webkit/appcache/appcache_database_unittest.cc (right):

http://codereview.chromium.org/8396013/diff/32026/webkit/appcache/appcache_da...
webkit/appcache/appcache_database_unittest.cc:345:
std::vector<AppCacheDatabase::NamespaceRecord> records;
On 2011/12/05 23:23:07, jennb wrote:
> Rename to fallbacks?

Done.

http://codereview.chromium.org/8396013/diff/32026/webkit/appcache/appcache_in...
File webkit/appcache/appcache_interfaces.h (right):

http://codereview.chromium.org/8396013/diff/32026/webkit/appcache/appcache_in...
webkit/appcache/appcache_interfaces.h:98: Namespace();
On 2011/12/05 23:23:07, jennb wrote:
> Should comment that default is Fallback namespace if type not specified?

Done.

http://codereview.chromium.org/8396013/diff/32026/webkit/appcache/appcache_st...
File webkit/appcache/appcache_storage_impl.cc (right):

http://codereview.chromium.org/8396013/diff/32026/webkit/appcache/appcache_st...
webkit/appcache/appcache_storage_impl.cc:1001: fallback_entry_ = AppCacheEntry(
On 2011/12/05 23:23:07, jennb wrote:
> Should this also be part of the if-fallback clause?

Done.

http://codereview.chromium.org/8396013/diff/32026/webkit/appcache/appcache_up...
File webkit/appcache/appcache_update_job_unittest.cc (right):

http://codereview.chromium.org/8396013/diff/32026/webkit/appcache/appcache_up...
webkit/appcache/appcache_update_job_unittest.cc:194: 
On 2011/12/05 23:23:07, jennb wrote:
> Extra blank line?

Done.

http://codereview.chromium.org/8396013/diff/32026/webkit/appcache/appcache_up...
webkit/appcache/appcache_update_job_unittest.cc:3054:
EXPECT_TRUE(cache->fallback_namespaces_[0] ==
On 2011/12/05 23:23:07, jennb wrote:
> Change to EXPECT_EQ?

for some reason EXPECT_EQ doesn't figure out the == operator?

[jennb, 2011-12-07 21:55:08]

Patch set 11 - looks alright so far. I'm trying to look periodically so the
diffs don't get too big. :-)

http://codereview.chromium.org/8396013/diff/42001/sql/connection.h
File sql/connection.h (right):

http://codereview.chromium.org/8396013/diff/42001/sql/connection.h#newcode289
sql/connection.h:289: // Internal helper for DoesTableExist and DoesIndexExist.
Does this return true if both Table and Index exist or if only one of them
exist?

http://codereview.chromium.org/8396013/diff/42001/webkit/appcache/appcache.h
File webkit/appcache/appcache.h (right):

http://codereview.chromium.org/8396013/diff/42001/webkit/appcache/appcache.h#...
webkit/appcache/appcache.h:141: 
Extra blank line?

[michaeln, 2011-12-08 01:38:59]

I've added unittests in snapshot 12.

http://codereview.chromium.org/8396013/diff/42001/sql/connection.h
File sql/connection.h (right):

http://codereview.chromium.org/8396013/diff/42001/sql/connection.h#newcode289
sql/connection.h:289: // Internal helper for DoesTableExist and DoesIndexExist.
On 2011/12/07 21:55:08, jennb wrote:
> Does this return true if both Table and Index exist or if only one of them
> exist?

The caller indicates which object type to look for in |type|. It only checks for
an object of that type under the given |name|.

http://codereview.chromium.org/8396013/diff/42001/webkit/appcache/appcache.h
File webkit/appcache/appcache.h (right):

http://codereview.chromium.org/8396013/diff/42001/webkit/appcache/appcache.h#...
webkit/appcache/appcache.h:141: 
On 2011/12/07 21:55:08, jennb wrote:
> Extra blank line?

Done (locally, not uploaded yet)

[michaeln, 2011-12-08 02:06:34]

http://codereview.chromium.org/8396013/diff/51001/webkit/appcache/appcache_st...
File webkit/appcache/appcache_storage_impl.cc (right):

http://codereview.chromium.org/8396013/diff/51001/webkit/appcache/appcache_st...
webkit/appcache/appcache_storage_impl.cc:838: NetworkNamespaceHelper*
network_namespace_helper);
hmmm... some compilers might complain about this use of a class defined in an
anon namespace in the interface of a class defined outside of that namespace?
Maybe i should move all of those helpers into the private section of this class
definition.

[michaeln, 2011-12-08 22:01:29]

fixed some lint errors in #13

[michaeln, 2011-12-13 00:20:26]

ping

[jennb, 2011-12-13 00:33:09]

LGTM

http://codereview.chromium.org/8396013/diff/55002/webkit/appcache/appcache_st...
File webkit/appcache/appcache_storage_impl_unittest.cc (right):

http://codereview.chromium.org/8396013/diff/55002/webkit/appcache/appcache_st...
webkit/appcache/appcache_storage_impl_unittest.cc:978: // MakeCacheAndGroup has
inserted  the default entry record already
extra space in sentence