[webkit-reviews] review granted: [Bug 23036] Implement AppCache fallback entries : [Attachment 26313] proposed patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Alexey Proskuryakov <ap at webkit.org>'s
request for review:
Bug 23036: Implement AppCache fallback entries
https://bugs.webkit.org/show_bug.cgi?id=23036

Attachment 26313: proposed patch
https://bugs.webkit.org/attachment.cgi?id=26313&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> +    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