AppCacheExecutableHandlers - parse manifest file and store the 'bit' indicating executable.

AppCacheExecutableHandlers - parse manifest file and store/retrieve the 'bit' indicating executable.

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

[michaeln, 2013-04-10 23:13:20]

can one of you ptal?

[Greg Billock, 2013-04-10 23:25:30]

https://codereview.chromium.org/13881003/diff/26001/webkit/appcache/appcache_...
File webkit/appcache/appcache_database.cc (right):

https://codereview.chromium.org/13881003/diff/26001/webkit/appcache/appcache_...
webkit/appcache/appcache_database.cc:693: // schemas, we use the high bit of
'type' field
Is this going to give us maintenance hassles later? I know a new schema will,
but how much of a price is this? I don't know namespace.type

https://codereview.chromium.org/13881003/diff/26001/webkit/appcache/appcache_...
File webkit/appcache/appcache_update_job.cc (right):

https://codereview.chromium.org/13881003/diff/26001/webkit/appcache/appcache_...
webkit/appcache/appcache_update_job.cc:896: if (it->is_executable)
Want to check flag here as well? Allows turn-off after registration, right?

https://codereview.chromium.org/13881003/diff/26001/webkit/appcache/manifest_...
File webkit/appcache/manifest_parser.cc (right):

https://codereview.chromium.org/13881003/diff/26001/webkit/appcache/manifest_...
webkit/appcache/manifest_parser.cc:262: if (type == L"return")
Can you put braces given the multi-line?

[alecflett, 2013-04-10 23:39:56]

https://codereview.chromium.org/13881003/diff/26001/webkit/appcache/appcache_...
File webkit/appcache/appcache_database.cc (right):

https://codereview.chromium.org/13881003/diff/26001/webkit/appcache/appcache_...
webkit/appcache/appcache_database.cc:693: // schemas, we use the high bit of
'type' field
On 2013/04/10 23:25:30, Greg Billock wrote:
> Is this going to give us maintenance hassles later? I know a new schema will,
> but how much of a price is this? I don't know namespace.type

Yeah, for instance we lose backwards compatibility in unusual ways - if this
gets backed out mid-milestone, this out is going to be ugly because old/new code
doesn't know that the high bit is set.

[michaeln, 2013-04-10 23:47:59]

https://codereview.chromium.org/13881003/diff/26001/webkit/appcache/appcache_...
File webkit/appcache/appcache_database.cc (right):

https://codereview.chromium.org/13881003/diff/26001/webkit/appcache/appcache_...
webkit/appcache/appcache_database.cc:693: // schemas, we use the high bit of
'type' field
On 2013/04/10 23:39:56, alecflett wrote:
> On 2013/04/10 23:25:30, Greg Billock wrote:
> > Is this going to give us maintenance hassles later? I know a new schema
will,
> > but how much of a price is this? I don't know namespace.type
> 
> Yeah, for instance we lose backwards compatibility in unusual ways - if this
> gets backed out mid-milestone, this out is going to be ugly because old/new
code
> doesn't know that the high bit is set.

This bugs me too :) We couldn't completely back everything one this goes in,
we'd have to leave the code that masks off the high bit forever.

Maybe we can simply not put this bit in this table for now? The entry.flags
field also has this new bit and that is what AppCacheUrlRequestJob is looking at
anyway. The bit in the Namespace struct would not survive roundtrips to and from
the db, but that seems ok for now. The important part is that the bit in Entry
does survive roundtrips. Still a little sloppy, but with less persistence.

https://codereview.chromium.org/13881003/diff/26001/webkit/appcache/appcache_...
File webkit/appcache/appcache_update_job.cc (right):

https://codereview.chromium.org/13881003/diff/26001/webkit/appcache/appcache_...
webkit/appcache/appcache_update_job.cc:896: if (it->is_executable)
On 2013/04/10 23:25:30, Greg Billock wrote:
> Want to check flag here as well? Allows turn-off after registration, right?

Don't think we need to here. This logic picks up the results of the parser very
shortly after the parser is done. The other check in AppCacheUrlRequestJob takes
care of turn-off later.

https://codereview.chromium.org/13881003/diff/26001/webkit/appcache/manifest_...
File webkit/appcache/manifest_parser.cc (right):

https://codereview.chromium.org/13881003/diff/26001/webkit/appcache/manifest_...
webkit/appcache/manifest_parser.cc:262: if (type == L"return")
On 2013/04/10 23:25:30, Greg Billock wrote:
> Can you put braces given the multi-line?

will do

[michaeln, 2013-04-11 00:00:28]

> Maybe we can simply not put this bit in this table for now?

Done

[michaeln, 2013-04-11 00:29:49]

Another possibility is mark in the db having been run with this experimental
flag set. Compare expected experimental flags with those in the db at init time.
If the set is different... nuke the db and start over.

So switching flags on and off would reset the entire db on every switch,
'overloading' existing columns in that world wouldn't be so dicey anymore.

[michaeln, 2013-04-11 17:42:05]

I think i like the exerimental on/off transition thing to keep us out of
trouble. I'll make a CL that can perform that transition separately, and then
use that gizmo here.

[michaeln, 2013-04-15 19:59:06]

Back to this one now...
* resets the db when the cmdline flag toggles between runs
* tucks the namespace.is_executable bit into the namespace table w/o changing
schemas (like the first patchset did)

[michaeln, 2013-04-16 20:18:59]

ping @alec | @gbillock

[michaeln, 2013-04-16 22:25:11]

https://chromiumcodereview.appspot.com/13881003/diff/49001/webkit/appcache/ap...
File webkit/appcache/appcache_database.cc (right):

https://chromiumcodereview.appspot.com/13881003/diff/49001/webkit/appcache/ap...
webkit/appcache/appcache_database.cc:963: int type_with_executable_bit =
statement->ColumnInt(2);
could make this const to make it more clear we don't fiddle the bits in this
value in the logic below

[alecflett, 2013-04-16 22:28:56]

lgtm with two totally minor suggestions, but nothing I feel strongly about.

https://chromiumcodereview.appspot.com/13881003/diff/49001/webkit/appcache/ap...
File webkit/appcache/appcache_interfaces.cc (right):

https://chromiumcodereview.appspot.com/13881003/diff/49001/webkit/appcache/ap...
webkit/appcache/appcache_interfaces.cc:76: is_executable(false) {
Seems like its worth keeping a single constructor around for explicitly
requiring is_executable = false

https://chromiumcodereview.appspot.com/13881003/diff/49001/webkit/appcache/ap...
File webkit/appcache/appcache_url_request_job.cc (right):

https://chromiumcodereview.appspot.com/13881003/diff/49001/webkit/appcache/ap...
webkit/appcache/appcache_url_request_job.cc:111: // TODO(michaeln): do something
different here with
perhaps LOG() here? (not sure what the policy is around logging these days)

[michaeln, 2013-04-16 22:43:01]

thnx for looking!

https://chromiumcodereview.appspot.com/13881003/diff/49001/webkit/appcache/ap...
File webkit/appcache/appcache_interfaces.cc (right):

https://chromiumcodereview.appspot.com/13881003/diff/49001/webkit/appcache/ap...
webkit/appcache/appcache_interfaces.cc:76: is_executable(false) {
thought about it, certainly if/when we decide to keep this exp in the code base
for the long haul would want to do that, but i expect we'll yank it at some
point so didn't do that to make it easier to yank

i could just not have the extra ctor at all and alter the callsite(s) to set the
member after construction

https://chromiumcodereview.appspot.com/13881003/diff/49001/webkit/appcache/ap...
File webkit/appcache/appcache_url_request_job.cc (right):

https://chromiumcodereview.appspot.com/13881003/diff/49001/webkit/appcache/ap...
webkit/appcache/appcache_url_request_job.cc:111: // TODO(michaeln): do something
different here with
On 2013/04/16 22:28:56, alecflett wrote:
> perhaps LOG() here? (not sure what the policy is around logging these days)

should be short-lived that this is empty, afaict also nobody reads that logging
really (wouldn't want to put the extra string data in a real release build
either)

[michaeln, 2013-04-17 18:50:47]

Committed patchset #6 manually as r194642 (presubmit successful).