[Webkit-unassigned] [Bug 22700] ApplicationCache should have size limit

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=22700





--- Comment #28 from Andrei Popescu <andreip at google.com>  2009-07-16 14:14:33 PDT ---
(In reply to comment #25)
> (From update of attachment 32709 [details])
> > +        https://bugs.webkit.org/show_bug.cgi?id=22700
> > +
> > +        https://lists.webkit.org/pipermail/webkit-dev/2009-May/007560.html
> 
> We usually put bug title after its URL in ChangeLogs.
> 

Added.

> > +    , m_estimatedSizeInDatabase(0)
> 
> I don't think that Database is the right word here - do we use it elsewhere in
> appcache interface? I'd prefer "OnDisk" or "InStorage".

True, we don't use it elsewhere. For consistency, I used 'InStorage".

> > +    // Note that there is no need to use a RefPtr here.
> > +    // The ApplicationCacheGroup instance is guaranteed
> > +    // to be alive when the timer fires since invoking
> > +    // the ChromeClient callback is part of its normal
> > +    // update machinery and nothing can yet cause it to
> > +    // get deleted.
> > +    ApplicationCacheGroup* m_cacheGroup;
> 
> You could make lines longer (I'm not sure if there's an official
> recommendation, but we certainly don't wrap at 80 characters). Due to its
> vertical size, this comment looks more important than it is - I usually expect
> that six-line comments describe architecture.
> 

Made longer.

> > +    // This flag is set immediately after the ChromeClient::reachedMaxAppCacheSize()
> > +    // callback is invoked as a result of the storage layer failing to save a cache
> > +    // due to reaching the maximum size of the application cache database file.
> > +    // This flag is used by ApplicationCacheGroup::checkIfLoadIsComplete() to decide
> > +    // the course of action in case of this failure (i.e. call the ChromeClient
> > +    // callback or run the failure steps).
> > +    // This flag is reset in ApplicationCacheGroup::checkIfLoadIsComplete().
> > +    bool m_calledReachedMaxAppCacheSize;
> 
> Ditto. I'm not sure if this even needs a comment in header.
>

Made longer (and a bit shorter, too) so I left it in.

> > @@ -35,6 +35,7 @@ ApplicationCacheResource::ApplicationCac
> >      : SubstituteResource(url, response, data)
> >      , m_type(type)
> >      , m_storageID(0)
> > +    , m_size(0)
> >  {
> >  }
> 
> Is resource size exact, or also an estimate?

Oh, it's an estimate as well. Renamed.

> > +     while (selectURLs.step() == SQLResultRow)
> > +        urls->append(selectURLs.getColumnText(0));
> 
> Too much indent before "while".
> 

Fixed.

> > +    if (!m_database.isOpen())
> > +       return false;
> 
> Too little before "return".
>

Fixed.

> 
> Anders may have more comments about the SQL part of this patch.

I also tried pinging Anders a while ago but without success...

Many thanks,
Andrei

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.