Bug 23036 - Implement AppCache fallback entries
Summary: Implement AppCache fallback entries
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Enhancement
Assignee: Alexey Proskuryakov
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2008-12-30 10:23 PST by Alexey Proskuryakov
Modified: 2009-01-02 01:31 PST (History)
0 users

See Also:


Attachments
proposed patch (36.97 KB, patch)
2008-12-30 10:47 PST, Alexey Proskuryakov
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexey Proskuryakov 2008-12-30 10:23:04 PST
Fallback namespaces: URLs, used as prefix match patterns, each of which is mapped to a fallback entry. Each namespace URL has the same origin as the manifest.
Comment 1 Alexey Proskuryakov 2008-12-30 10:23:30 PST
<rdar://problem/6451666>
Comment 2 Alexey Proskuryakov 2008-12-30 10:47:28 PST
Created attachment 26313 [details]
proposed patch
Comment 3 Darin Adler 2008-12-30 11:02:06 PST
Comment on attachment 26313 [details]
proposed patch

> +    if (!cache)
> +        cache = applicationCache();
> +    if (!cache)
> +        return false;

I suggest nesting the second if statement inside the first.

> +                    data.clear();
> +                    data.append(resource->data()->data(), resource->data()->size());

We recently changed clear() so that it actually shrinks the capacity of the vector. This makes this an inefficient algorithm since it involves a free and subsequent malloc. On the other hand, if the data is going to be kept around for a while, then maybe it's the right thing to do. Another possibility is to call shrink(0) instead of clear(). Or to call resize() and then memcpy() instead of append().

> +    unsigned fallbackCount = m_fallbackURLs.size();
> +    for (unsigned i = 0; i < fallbackCount; ++i) {

We normally use size_t to iterate vectors. I'm concerned that using a vector won't scale well if the number of URLs gets large.

> +    void setFallbackURLs(const Vector<std::pair<KURL, KURL> >&);

I think this vector type could use a typedef. FallbackURLVector maybe?

> +    unsigned fallbackCount = manifest.fallbackURLs.size();
> +    for (unsigned i = 0; i  < fallbackCount; ++i)

Again, size_t.

> +        unsigned newestCacheID = (unsigned)statement.getColumnInt64(2);

> +        group->setStorageID((unsigned)statement.getColumnInt64(0));

Lets use C++ casts instead of C casts. And for integer truncation, we normally don't cast at all -- we use local variables and rely on the compiler allowing that without a warning. Not a great approach, I guess, but I really believe in keeping the casts down to a minimum and as clear as possible. Maybe we should define our own truncation_cast function template?

> +        unsigned fallbackCount = fallbackURLs.size();
> +        for (unsigned i = 0; i < fallbackCount; ++i) {

size_t again.

> +                namespaceURL.setRef(String());

Makes me wish for a clearRef() function.

r=me
Comment 4 Alexey Proskuryakov 2009-01-02 01:31:20 PST
Committed as <http://trac.webkit.org/changeset/39546>.